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

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

Closed
jmuzina opened this issue Jul 24, 2024 · 8 comments · Fixed by #5259
Closed

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

jmuzina opened this issue Jul 24, 2024 · 8 comments · Fixed by #5259
Assignees
Labels
Documentation 📝 Documentation changes or updates

Comments

@jmuzina
Copy link
Member

jmuzina commented Jul 24, 2024

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.

@jmuzina jmuzina added the Documentation 📝 Documentation changes or updates label Jul 24, 2024
Copy link

Thank you for reporting us your feedback!

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

This message was autogenerated

@pastelcyborg pastelcyborg self-assigned this Jul 24, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Jul 24, 2024

@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
Copy link
Contributor

Hey, that looks pretty good! Couple questions:

  • Are there any downsides you're aware of?
  • What made you choose js-beautify? Were there any alternatives you considered?

@jmuzina
Copy link
Member Author

jmuzina commented Jul 25, 2024

@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
Copy link
Contributor

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
Copy link
Member Author

jmuzina commented Jul 25, 2024

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
Copy link
Member Author

jmuzina commented Jul 25, 2024

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
Copy link
Member Author

jmuzina commented Jul 26, 2024

@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

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

Successfully merging a pull request may close this issue.

2 participants