fix: Add icon granularity to HTML code generation #315
Conversation
Rendering is conditional based on whether or not the icon set is using an array, if not it returns the value of the icon set. For instance, `icons.favicons === false` would not generate HTML for favicons. Also refactored a lot of repeated (copy-pasted / line duplicated) code.
|
I will, as soon as all the test stop relying on arbitrary whitespace constraints. |
|
@Voltra What is the next step for us? |
|
Snapshots/fixtures should be rewritten to avoid the use of specific amounts of whitespace per item (only 1 per attribute). I also recommend the use of self-closing tags (i.e. Once all the whitespace issues are gone, I can proceed with tests for granularity on the HTML code generation level. |
Do you want to send a fix? |
test/snapshots/array.test.js.md
Outdated
| '<link rel="apple-touch-icon" sizes="167x167" href="/apple-touch-icon-167x167.png"/>', | ||
| '<link rel="apple-touch-icon" sizes="180x180" href="/apple-touch-icon-180x180.png"/>', | ||
| '<link rel="apple-touch-icon" sizes="1024x1024" href="/apple-touch-icon-1024x1024.png"/>', | ||
| '<meta name="apple-mobile-web-app-capable" content="yes"/>', |
There was a problem hiding this comment.
I think we should avoid / here and introduce the html5 option, but we can do it in separate PR
There was a problem hiding this comment.
Although they are considered useless as per the HTML5 standard, they still carry the meaning of being closed immediately with no content
There was a problem hiding this comment.
Let's avoid them anyway, I'm sure users will create a problem about this.
|
The tests pass for the HTML generation |
|
@Voltra What is the next? |
|
@Voltra has the html code generation been merged and published for? Latest release seem to be "on 21 Jul 2020" so I would assume that no because you would like to fix manifest generation as well? |
Changes
Why
From #312 it has been brought to my attention that HTML codegen was not affected by the newly added granularity.
This aims to only generate HTML for icons that were specified.
How
With a few helpers, check if the options for the working icon set is an array and if it is, only generate the selected options. If it's not an array, proceed as usual.
Feedback
It modified a lot of the HTML codegen structure to allow for better maintainability and flexibility (e.g. adding new icons). This also clears a lot of "code smells" were a lot of lines were simply duplicates with small adjustments. Also brought to the light that there were a lot of strange decision regarding whitespaces (especially between HTML attributes for some reason) as well as a bad habit of not autoclosing tags (i.e. leaving them open as if they were opening tags).
You can take a look at config/html.js from line 40 to line 71, especially the
hrefattributes and end of tags to know what I'm talking about.Nota Bene
I will not in any way shape or form alter the tests and/or fixtures/snapshots that rely on mentioned strange decisions.
As such, some tests fail (due to missing whitespaces).
Improvements in code quality makes it trivial to implement #289 on top of it.