canonical / vanilla-framework

From community websites to web applications, this CSS framework will help you achieve a consistent look and feel.
https://vanillaframework.io
GNU Lesser General Public License v3.0
819 stars 167 forks source link

Fix inconsistent whitespace & indentation in macro-generated examples markup #5245

Closed jmuzina closed 1 month ago

jmuzina commented 1 month ago

Recent work on macros in PRs such as #5213 , #5242 , and #5212 revealed some problems rendering markup generated by macros within example code snippets. They often have a large amount of excess whitespace and incorrect tabbing.

image

Using Jinja whitespace control helps a lot with eliminating excess newlines, but does not help with indentation.

We should find a way to make the indentation of our HTML (and possibly JS) for examples consistent. As a stretch goal, it would be nice to not have to worry as much about handling newlines, and have that handled for us as well.

syncronize-issues-to-jira[bot] commented 1 month ago

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/WD-13629.

This message was autogenerated

jmuzina commented 1 month ago

@pastelcyborg I briefly explored solving the indentation portion of this problem with js-beautify. Have a look at the indent-example-html-js branch.

Here are the changes.

Before: image

After: image

pastelcyborg commented 1 month ago

Hey, that looks pretty good! Couple questions:

jmuzina commented 1 month ago

@pastelcyborg

Are there any downsides you're aware of?

  1. As far as I can tell, there is no Typescript support. This isn't a problem for us currently but it would be nice to have static type checking if we decide to build Vanilla JS with TS later down the line.
  2. I couldn't find an easy way to include their minified JS with Node. I don't think they include their minified JS in the node package (I found beautify.js and beautify-html.js, and beautifer.min.js in their dist, but this is confusing when you read their documentation for which scripts to include, which suggests there should be .min versions of all scripts).
  3. I couldn't easily strip excess newlines / whitespace with js-beautify, so in its current state you still need to use Jinja whitespace control to get rid of extra newlines, then handle indentation with js-beautify. It's possible this could be accomplished by passing args into js_beautify and html_beautify. Note that these arg names are for the CLI, and their JS equivalents are less documented, but I was getting code completion hints when using snake case for option names, so it seems you can use snake case to pass in options in JS (i.e. {brace_style: 'collapse'} instead of --brace-style collapse)

What made you choose js-beautify? Were there any alternatives you considered?

I was initially considering indent.js but it's no longer maintained. I then found js-beautify and was particularly impressed by its Snyk score and widespread usage (it has ~4M downloads per week on npmjs)

pastelcyborg commented 1 month ago

I think those are all reasonable conclusions/accommodations.

It does look like there are TS types available: https://www.npmjs.com/package/@types/js-beautify. However, I honestly would probably just use the CDN for this, as it's completely unrelated to any of our library/site business logic and is solely used to improve the presentation of code samples for the end user, so I don't see a real need to include it as part of our JS bundle. Even if the library fails to load or execute for some reason, it would have minimal bearing on anything, other than slightly less pleasant code samples to look at - we'll continue to use Jinja whitespace stripping to its fullest extent anyway, so that should get us 80% of the way there; js-beauty is really just for indentation.

@bartaz Thoughts?

jmuzina commented 1 month ago

I honestly would probably just use the CDN for this, as it's completely unrelated to any of our library/site business logic and is solely used to improve the presentation of code samples for the end user, so I don't see a real need to include it as part of our JS bundle.

This is a good argument - I initially wanted to use NPM simply because I like using it to manage as many dependencies as possible, but I think in this case it's OK if we use minified scripts from the CDN

jmuzina commented 1 month ago

If we do go with this tool, we should adjust the options so that the indent size is 2, instead of the default 4. That seems to be the Canonical standard.

Here's a usage example.

jmuzina commented 1 month ago

@pastelcyborg I hope you don't mind, I made a draft PR (under canonical repo so we can all contribute) with my current implementation, feel free to adjust it!

5259