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

USWDS-Site - Spacing token - Repair converted tokens #1949

Merged
merged 15 commits into from Feb 24, 2023
Merged

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Dec 16, 2022

Summary

Related issue

Closes #1934

Preview links

Problem statement

On the spacing tokens page, the token 5 is incorrectly linked to the 0.5 number equivalent. 0.5 should instead be linked to token '05'. This appears to happen because of confusion with the leading zero and quotes in '05' – something is getting lost in translation between the files that prevents '05' from receiving any converted value. 5 receives the converted value instead.

image

On the spacing unit page, we need to both:

  1. Prevent the 5 token from receiving the 0.5 number equivalent AND
  2. Associate the '05' token with the 0.5 number equivalent

Additionally, the following pages also need to break the association between the 5 token and the 0.5 conversion:

Solution

The converted assignment in is_number.html consistently ignored the leading zero in the '05' token and would assign the converted value to 5 instead. Attempts to explicitly declare '05' as a string to force the leading zero were not successful.

As a workaround, this PR creates an explicit conversion value for '05' in spacing.yml. Declaring an explicit value here removes the confusion that is currently happening between files.

The spacing unit page appears to be the only instance of '05' token that is problematic.

Screenshots

Final result:

image

Testing and review

To review:

  • Confirm that the 5 token does not have the 0.5 number equivalent on any of the preview pages.
  • Confirm that the '05' has the 0.5 equivalent on the spacing tokens page.
  • Confirm that all pages that use {{ converted }} display correct values.

@amyleadem
Copy link
Contributor Author

@mejiaj This PR creates a workaround for the confusion that happens with the leading zero and quotes between files. I tried to prevent the confusion with different methods of making '05' an explicit string in yaml, but I can't seem to make it all work. If you see alternate methods, please let me know!

@amyleadem amyleadem marked this pull request as ready for review December 21, 2022 19:17
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

I can see the benefit of having the ability to override a conversion. We could also approach this from another angle.

The issue seems to be is_number.html and how it treats the 05 value:
https://github.com/uswds/uswds-site/blob/aa4c540b646af56c01250e1dc5f3ef8fb2191446/_includes/tokens/is_number.html#L22-L30

We could add an additional check that would give us the number or the converted value, instead of both (because we're already checking in the template):

{% if is_number %}
  {% assign converted = false %}
{% else %}
  {% assign conversion =
    site.data.tokens.conversion
    | where: 'token', item.token %}

  {% if conversion[0].number %}
    {% assign converted = conversion[0].number %}
  {% endif %}
{% endif %}

This resulted in a table that no longer has 0.5 as a value of 5:
image

What do you think @amyleadem @thisisdano?

@amyleadem
Copy link
Contributor Author

@mejiaj I attempted to implement your suggested solution. It fixed the false association with the 5 token, but I still had trouble getting the system to recognize that 0.5 should be paired with 05. Any tips?

image

@amyleadem amyleadem requested a review from mejiaj January 24, 2023 19:28
@mejiaj
Copy link
Contributor

mejiaj commented Feb 6, 2023

@mejiaj I attempted to implement your suggested solution. It fixed the false association with the 5 token, but I still had trouble getting the system to recognize that 0.5 should be paired with 05. Any tips?

image

I think that's what _data/tokens/conversion.yml was intended for. That might be why '05' was included originally. There's definitely something strange going on with the logic in is_number.html.

It looks like '05' is not converted and not a number, based on the logic in template:

// spacing-units.md
{% if converted %}
  <code>{{ converted }}</code>
{% endif %}
{% if is_number %}
  <code>{{ item.token }}</code>
{% else %}
  // '05' returns false for both above and just prints out as token
  <code>'{{ item.token }}'</code> 
{% endif %}

Comment on lines +32 to +34
{% if is_number %}
{% assign converted = false %}
{% else %}
Copy link
Contributor Author

@amyleadem amyleadem Feb 24, 2023

Choose a reason for hiding this comment

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

Checking if the item is a number first prevents the 5 token from incorrectly receiving a converted value.

@@ -7,6 +7,7 @@ positive:
small:
- token: '05'
value: 4px
conversion: 0.5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an explicit equivalent number value here because it could not recognize the '05' token when assigning converted in is_number.html

@@ -2,6 +2,19 @@ title: Spacing tokens
type: token
changelogURL:
items:
- date: 2022-12-19
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to update this date prior to merge

@amyleadem
Copy link
Contributor Author

@mejiaj
I incorporated your logic to check for is_number before assigning converted. Thanks for that suggestion.

I was not able to get converted to recognize the leading zero in '05'. For this reason, I believe the best path forward is to set an explicit conversion value, which is found in this PR. Let me know if you have any questions or concerns.

Also - the checks are currently failing because snyk ignore needs to be updated. The forthcoming snyk fix is in this PR: #2013

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Nice work fixing this issue! The output table on the spacing units page looks good.

Just a minor question on the PR date in changelog. Otherwise LGTM!

@@ -2,6 +2,19 @@ title: Spacing tokens
type: token
changelogURL:
items:
- date: 2022-12-19
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be merge date right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in ed3e647

@@ -56,6 +56,8 @@ Any spacing value in utilities or component CSS should use the following spacing
<span class="text-no-wrap padding-right-4">
{% if converted %}
<code>{{ converted }}</code>,
{% elsif item.conversion %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just suggest we add a note for what this means. That way we can understand the logic flow in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments in b4b7ad7

@mejiaj mejiaj merged commit 086b7a7 into main Feb 24, 2023
@amyleadem amyleadem deleted the al-spacing-token-05 branch July 17, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS-Site - Spacing tokens: Incorrect value for token 0.5
2 participants