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

feat: Publish & Document Jinja Macros #5342

Merged
merged 53 commits into from
Oct 4, 2024
Merged

feat: Publish & Document Jinja Macros #5342

merged 53 commits into from
Oct 4, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Sep 11, 2024

Done

Feature branch PR for publishing & documenting Jinja macros

QA

  • See QA instructions on the above PRs if you want to re-QA the feature PR.

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

@jmuzina jmuzina added the Feature 🎁 New feature or request label Sep 11, 2024
@webteam-app
Copy link

@jmuzina jmuzina added the Priority: High Should be addressed within current iteration label Sep 19, 2024
@jmuzina jmuzina added Review: QA needed Review: Code needed Review: Percy needed This PR needs a review of Percy for visual regressions Review: Percy +1 and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Sep 19, 2024
@jmuzina jmuzina changed the title wip: feat: Publish & Document Jinja Macros feat: Publish & Document Jinja Macros Sep 19, 2024
@jmuzina jmuzina marked this pull request as ready for review September 19, 2024 19:12
@bartaz
Copy link
Member

bartaz commented Sep 25, 2024

Looks good overall.

One downside is that the start of the page is very table-heavy, full of props you need to parse (before you even see what the pattern actually looks like).

image

I'd personally suggest moving the "Jinja Macro" section to the bottom of the page. I know we want devs to have access to this spec, but to me it seems like having the examples first would give a better overview of the component. They are gonna copy the snippet in 90% of cases anyway. Then, if they want to have a look at the reference, they can find it on the page.

Thoughts?

@jmuzina
Copy link
Member Author

jmuzina commented Sep 25, 2024

One downside is that the start of the page is very table-heavy, full of props you need to parse (before you even see what the pattern actually looks like). I'd personally suggest moving the "Jinja Macro" section to the bottom of the page. I know we want devs to have access to this spec, but to me it seems like having the examples first would give a better overview of the component. They are gonna copy the snippet in 90% of cases anyway. Then, if they want to have a look at the reference, they can find it on the page. Thoughts?

@bartaz Good point. I've moved it to just above the "import" section as I feel it might be a bit disjointed to read examples -> import instructions -> Jinja API docs

We could also move the Jinja API docs to their own tab / sub-page, any thoughts on putting it at bottom of page vs on a separate sub-page? @bartaz @pastelcyborg @advl

@pastelcyborg
Copy link
Contributor

I'd think moving the Jinja API docs to a separate tab would fall under the decision we made about moving the Jinja examples/code samples to a separate tab - that is, we decided not to do that because it would increase complexity and make the docs more convoluted.

I think the structure as represented right now is acceptable. The only change I would possibly make is moving the macro API docs after the Jinja import instructions, but it's really not a big deal either way.

@apollo13
Copy link

apollo13 commented Oct 1, 2024

Hi, this looks interesting. The slot checking in the macro calling is a bit ugly imo -- the following jinja issue has an alternative that is nicer I think: pallets/jinja#482 (comment)

This would kinda look like this:

{% macro vf_hero(
  title_text,
  subtitle_text='',
  layout="fallback",
  is_split_on_medium=false
) -%}
  {% set has_subtitle = subtitle|trim|length > 0 %}
  {% set has_subtitle = subtitle_text|trim|length > 0 %}
  {% set description_content = caller(description=True) if 'description' in caller.arguments else '' %}
  {% set cta_content = caller(cta=True) if 'cta' in caller.arguments else '' %}

(see the last lines) and then used in call like this:

{% call(description, cta) vf_hero(…) %}
{% if description %}
…
{% else if cta %}
…
{% endif %}
{% endif %}

Personally I think this is more readable.

bartaz
bartaz previously approved these changes Oct 3, 2024
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM

@jmuzina
Copy link
Member Author

jmuzina commented Oct 3, 2024

Hi, this looks interesting. The slot checking in the macro calling is a bit ugly imo -- the following jinja issue has an alternative that is nicer I think: pallets/jinja#482 (comment)

Hi @apollo13 , thanks for your feedback! We've discussed it as a team and decided to stick with the current approach, at least for now - we've established this style as a team standard for this opening round of macros, but will keep it in mind for the future. It's nice to have some feedback from someone who is so familiar with these!

@jmuzina jmuzina added Review: Percy needed This PR needs a review of Percy for visual regressions and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Oct 3, 2024
@apollo13
Copy link

apollo13 commented Oct 3, 2024 via email

@jmuzina jmuzina merged commit e2d9aa7 into main Oct 4, 2024
12 checks passed
@jmuzina jmuzina deleted the feature/expose-macros branch October 4, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature 🎁 New feature or request Priority: High Should be addressed within current iteration Review: Code +1 Review: Percy +1 Review: QA +1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants