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

WIP: Experiment with converting Tiered List Macro to use variant param (DO NOT MERGE) #5304

Closed

Conversation

pastelcyborg
Copy link
Contributor

@pastelcyborg pastelcyborg commented Aug 15, 2024

The intent of this PR is to explore changing our Macro API to use a unified variant param for layout permutations, rather than multiple, inconsistent string/boolean params. There are a few goals for doing this:

  • Provide unified, predictable param across all our Macros
  • Allow for much more flexible param that will give us the option of easily adding new variants as design specs change
  • Allow for easier documentation on docs site, explaining the param and its effects in a clear manner

Done

  • Converted tiered list macro to use new variant param instead of multiple boolean params
  • Added new variant documentation table to docs page

Fixes WD-14207

QA

  • Open demo
  • Read docs for Macro and new variant param

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.

@webteam-app
Copy link

@pastelcyborg pastelcyborg changed the title Tiered list variants param Experiment with converting Tiered List Macro to use variant param (DO NOT MERGE) Aug 15, 2024
@pastelcyborg pastelcyborg marked this pull request as ready for review August 15, 2024 19:49
Copy link
Member

@jmuzina jmuzina left a comment

Choose a reason for hiding this comment

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

While this does make adding new variants a lot easier, I'm not 100% sold on this integer-based approach due to its effects on our DX & UX. I know we'll end up having a wider team discussion about this, so just leaving some thoughts in the comments below.

I wonder if there is some compromise we could come to that makes our parameter juggling a bit more manageable while also not harming DX or UX. Perhaps we could keep this macro with one layout parameter (either a string or an integer), then add macros that are more user-focused and more semantically meaningful? For example:

{% macro tiered_list_full_width_desktop() %}
  {% call tiered_list(2) %}
    // slots added here
  {% endcall %}
{% endmacro %}

@@ -6,7 +6,7 @@
{% block standalone_css %}patterns_all{% endblock %}

{% block content %}
{%- call(slot) tiered_list(is_description_full_width_on_desktop=false, is_list_full_width_on_tablet=false) -%}
{%- call(slot) tiered_list(variant=3) -%}
Copy link
Member

Choose a reason for hiding this comment

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

This is a great example of how much simpler this approach makes calling these macros.

Copy link
Member

Choose a reason for hiding this comment

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

While "simpler" it is completely obscured. What does the "3" mean? How would anyone know what it means and what value to choose? Would designers and developers understand them in the same way? What if we need to add another slightly different version of existing one? Throwing in just another number (like variant=5) if that's a new modification for responsiveness of variant=1 seems weird and confusing.

With is_description_full_width_on_desktop=false, is_list_full_width_on_tablet=false I at least know more or less what options I'm setting. With variant=3 I know nothing.

Developers are likely used to using words for naming Vanilla options, because they have to do it in CSS using class names. This way, the name of a variant is at least somewhat descriptive. This also allows us to have separate flags if there are independent options to change, rather than hiding everything behind a number.

I guess I wouldn't be so opposed to that if instead of numbers we would use some more descriptive names, like "full-width" "full-width-on-large" "full-width-on-medium" … but, with macros we can't really enforce the values with any enum, so it becomes a free string (which is quite error prone). The named parameters at least give us a bit of control over what is being passed to the macro.

Overall, I wonder why do we want to change this? Has current approach turned out to be so complex people don't know how to use it? Is the proposed one actually better in this regard?

Comment on lines +46 to +49
| `1` | [50/50 on desktop with description](#5050-on-desktop-with-description) | Yes | Full-width | 50/50 |
| `2` | [50/50 on tablet without description](#5050-on-tablet-without-description), <br>[50/50 on tablet with description](#5050-on-tablet-with-description) | No | 50/50 | Full-width |
| `3` | [50/50 with description](#5050-with-description) | Yes | 50/50 | 50/50 |
| `4` | [Full-width without description](#full-width-without-description), <br>[Full-width with description](#full-width-with-description) | No | Full-width | Full-width |
Copy link
Member

Choose a reason for hiding this comment

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

While this table is perfectly clear, I'm still not 100% comfortable with using integers to represent layouts for readability reasons.

Comment on lines +9 to 10
{%- call(slot) tiered_list(variant=4) -%}
{%- if slot == 'title' -%}
Copy link
Member

Choose a reason for hiding this comment

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

My readability concern is that the user can't tell anything about what variant=4 means just by looking at this macro call

@@ -22,7 +18,7 @@
<hr class="u-hide--small">

{% if has_description == true -%}
{%- if is_description_full_width_on_desktop == true -%}
{%- if variant in [2, 4] -%}
Copy link
Member

Choose a reason for hiding this comment

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

Relying on integers also makes our DX here a little worse, as the developer has to check the documentation table to translate these integers to their meaning. However, we already do this for proper CSS class usage, so it's not necessarily a deal-breaker.

{% macro tiered_list(
is_description_full_width_on_desktop=true,
is_list_full_width_on_tablet=true) -%}
{% macro tiered_list(variant=1) -%}
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the user passes in an invalid variant? Is it a valid default layout?

@jmuzina jmuzina mentioned this pull request Aug 20, 2024
3 tasks
@pastelcyborg pastelcyborg marked this pull request as draft September 17, 2024 19:57
@pastelcyborg pastelcyborg changed the title Experiment with converting Tiered List Macro to use variant param (DO NOT MERGE) WIP: Experiment with converting Tiered List Macro to use variant param (DO NOT MERGE) Sep 17, 2024
@bartaz
Copy link
Member

bartaz commented Oct 3, 2024

Thanks @pastelcyborg but we are not gonna do it!

@bartaz bartaz closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants