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

Try out standard indentation #57

Merged
merged 12 commits into from Jun 1, 2023
Merged

Try out standard indentation #57

merged 12 commits into from Jun 1, 2023

Conversation

jgerigmeyer
Copy link
Member

@netlify
Copy link

netlify bot commented May 30, 2023

Deploy Preview for sass-site-oddbird ready!

Name Link
🔨 Latest commit b1dcf05
🔍 Latest deploy log https://app.netlify.com/sites/sass-site-oddbird/deploys/64778dd673c8f60008cd37c9
😎 Deploy Preview https://deploy-preview-57--sass-site-oddbird.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

View diff of compiled files (may take a few minutes): https://github.com/oddbird/sass-site-built/compare/main..indentation

@jgerigmeyer jgerigmeyer marked this pull request as ready for review May 31, 2023 15:28
@jgerigmeyer
Copy link
Member Author

@jerivas @sanajaved7 I think this seems to work well, so that we can use more typical indentation for paired shortcodes (e.g. {% compatibility %}, {% codeExample %}, etc). If the approach looks okay to you two, I'll apply it globally.

Note that it doesn't solve the problem of codeExample not playing well with markdown -- I looked into that again, but don't think there's an easy way around the issue where code examples include multiple newlines (\n\n), which markdown turns into <p> tags.

Copy link
Member

@jerivas jerivas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Makes it way easier to browse a file and identify the blocks of content 👍

@jgerigmeyer jgerigmeyer requested a review from jerivas May 31, 2023 16:50
@jgerigmeyer
Copy link
Member Author

@jerivas I think I converted the remaining pages, so this is ready for final review ✔️

@@ -69,7 +69,7 @@ table_of_contents: true

{{ '## `@media`' | markdown }}

{% # Arguments are (in order): `dart`, `libsass`, `node`, `ruby`, optional feature name, additional details within %}
{% # Arguments are (in order): `dart`, `libsass`, `node`, `ruby`, optional feature name, `useMarkdown` boolean, additional details within %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but what do you think about changing {% compatibility %} to accept string arguments instead, and parse them in the TS implementation into a proper object:

{% compatibility 'dart:"1.11.0"', 'libsass:false', 'ruby:false', 'feature:"Range Syntax"', 'useMarkdown:false' %}

Advantages:

  • We can get rid of the comment before each invocation
  • No need to specify null for unused arguments, not including them should be enough
  • If 11ty lands support for kwargs, we can migrate to them with a regex that removes the single quotes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it! I'll try that out in this another PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in #59.

Copy link

@sanajaved7 sanajaved7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! so much easier to read

@jgerigmeyer jgerigmeyer merged commit 8b6aee9 into main Jun 1, 2023
9 checks passed
@jgerigmeyer jgerigmeyer deleted the indentation branch June 1, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants