-
Notifications
You must be signed in to change notification settings - Fork 166
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
Fix inconsistent whitespace & indentation in macro-generated examples markup #5245
Comments
Thank you for reporting us your feedback! The internal ticket has been created: https://warthogs.atlassian.net/browse/WD-13629.
|
@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. |
Hey, that looks pretty good! Couple questions:
|
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) |
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? |
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 |
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. |
@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! |
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.
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.
The text was updated successfully, but these errors were encountered: