Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes per tag PoC #117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Attributes per tag PoC #117

wants to merge 1 commit into from

Conversation

mkopylec
Copy link

Build is not passing, just take a look at the code.
This change will add an ability to use API in the following way:

        a().attr(AAttribute.HREF, "http://example.com");
        area().attr(AreaAttribute.TARGET, "_blank");

This type safe way to set attributes will force developer to use only proper attributes for HTML tags.
We could also remove methods that allow attribute names as Strings.
Also Attrs constants can be removed and changed to enums like AAttribute.
The predefined attribute accessors from Tag should also be removed because they allow to set improper attributes for HTML tags.

@tipsy
Copy link
Owner

tipsy commented May 18, 2018

If we're really doing this, then I think it should be withXyz, where only the valid attributes for that tag type are available. Using enums can cause some confusion ("What is an AAttribute.CLASS? How is it different from an AreaAttribute.CLASS?)", and it also makes the code a lot more verbose.

a().withHref("http://example.com"); 
area().withHref("http://example.com") // doesn't compile

.attr() can then be used for custom and/or unsupported attributes.

@mkopylec
Copy link
Author

I like that form too, but there will be a lot more code to implement (every tag arrtibute will be a delegate to base class).
But if you are OK with it I could create such a PR.

@tipsy
Copy link
Owner

tipsy commented May 18, 2018

I know, I am bit worried about the amount of code, but I think it's a better solution. Create a poc and we'll see how it looks.

@mkopylec
Copy link
Author

I have updated the PR

@tipsy
Copy link
Owner

tipsy commented May 18, 2018

Yeah, we can't maintain this. We need to find a way to do this more cleverly (interfaces?), or just generate the code 🤔

@mkopylec
Copy link
Author

My first thought was a code generator

@tipsy
Copy link
Owner

tipsy commented May 19, 2018

Maybe annotations can be used? I've never used them myself, but I know a lot of libraries use annotations to generate code.

@mkopylec
Copy link
Author

For sure there are plenty of java code generators.
I'll check them out and see if any is suitable for out case.

@rupert-madden-abbott
Copy link
Contributor

Have you guys seen how kotlinx.html has done this: https://github.com/kotlin/kotlinx.html/wiki/Getting-started

Essentially, they have got some source files representing relevant parts of the HTML 5 spec: https://github.com/Kotlin/kotlinx.html/tree/master/generate/src/main/resources. I imagine this either comes from an IDE (Eclipse?) or Mozilla? This means they don't need to maintain this datasource themselves.

Then their generate module parses this and coverts it into the Kotlin library.

Not sure if this PR is still being worked on?

This seems like a massive but hugely useful change. @tipsy do you want your project to go down this route or do you think this would be better attempted in a different project?

If we were to do it in this project, I would try and do so in small steps. So first thing would be to parse the datasource and then see if I could get it to generate just the methods that already exist in TagCreator.

JavaPoet seems to be a highly recommended code generator.

@tipsy
Copy link
Owner

tipsy commented Aug 21, 2018

This seems like a massive but hugely useful change. @tipsy do you want your project to go down this route or do you think this would be better attempted in a different project?

I wouldn't mind, I think it's a good approach. I have other projects which I consider higher priority though, so it would have to be up to the community to implement it. I can help with discussion/code reviews.

@pointbazaar
Copy link
Contributor

is this still being worked on?
I am using J2HTML on some of my projects and would like it to have more interfaces/classes to support writing html in a more typesafe manner. The resulting code would not allow for certain mistakes to be made which could be rendered differently in different browsers. This change would make the project way more valuable for people like me who have to make sure that
the page renders correctly in all browsers. I would be willing to implement it in small steps if no one else is working on it and it's still wanted for this project. (meaning my hypothetical PR would be reviewed)

@tipsy
Copy link
Owner

tipsy commented Aug 13, 2020

No one is working on this issue. I'm happy to review if the PRs are easily reviewable :)

@pointbazaar
Copy link
Contributor

PR is out !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants