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

chore: add cssrem VSCode extension recommendation #10300

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

jcfranco
Copy link
Member

Related Issue: N/A

Summary

Adds the cssrem extension to display unit conversions within the editor. This will help avoid adding comments for unit conversions (see #10179 (comment)).

@jcfranco jcfranco added the skip visual snapshots Pull requests that do not need visual regression testing. label Sep 13, 2024
@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Sep 13, 2024
@alisonailea
Copy link
Contributor

Per my comment on #10179, I don't think this needs to be added as it's solving for a short-term problem. Perhaps we should reprioritize the switch to rems in the new spacing tokens so it can be used sooner rather than later @SkyeSeitz @ashetland

@jcfranco
Copy link
Member Author

Noted, but I’d really like to avoid the noise and overhead of manually adding conversion comments. We could treat this as a temporary extension and remove it once the rem switch is in place.

@alisonailea
Copy link
Contributor

With this extension, all these tokens will have their values replaced with pixels. When the time comes to find/replace these static values with spacing tokens it's going to be a lot easier to find these with the "noise" than to try and filter out all the other pixel values that currently exist for other reasons.

@jcfranco
Copy link
Member Author

jcfranco commented Sep 13, 2024

With this extension, all these tokens will have their values replaced with pixels.

There’s a miscommunication. I wasn't proposing this extension to switch units, but rather to display the conversion within the editor. To be clear, we would keep rem values, and this would just help display their pixel conversion.

The file from the linked discussion isn’t the only place where this occurs.

When the time comes to find/replace these static values with spacing tokens it's going to be a lot easier to find these with the "noise" than to try and filter out all the other pixel values that currently exist for other reasons.

If we’re adding these comments for tracking and future replacement, let’s make sure the comments are clear about their purpose. /* 1337px */ isn’t clear in this regard. If there’s a spacing token issue, we could also use the comment to track future token replacements. That said, I agree the former is the simpler option for the switch.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Sep 21, 2024
@jcfranco jcfranco marked this pull request as ready for review September 25, 2024 19:39
@jcfranco jcfranco requested a review from benelan as a code owner September 25, 2024 19:39
@jcfranco jcfranco removed the Stale Issues or pull requests that have not had recent activity. label Sep 25, 2024
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

👍

@jcfranco jcfranco merged commit e95061d into dev Oct 3, 2024
13 checks passed
@jcfranco jcfranco deleted the jcfranco/add-cssrem-extension-recommendation branch October 3, 2024 03:19
@github-actions github-actions bot added this to the 2.13.1 patch milestone Oct 3, 2024
benelan added a commit that referenced this pull request Oct 7, 2024
* origin/dev: (230 commits)
  chore: release next
  chore(sort-handle): add messages (#10474)
  feat(accordion-item): stretch slotted actions to fill its height (#9250)
  chore: release next
  feat(dialog, modal, popover, input-date-picker,  input-time-picker, sheet): support stacked component sequential closing with escape (#9231)
  chore: remove commented-out code (#10478)
  chore: add cssrem VSCode extension recommendation (#10300)
  docs(accordion-item): fix deprecation tag (#10479)
  chore: release next
  feat(stepper-item): update component's active state background color. (#10475)
  refactor: use `requestAnimationFrame` to replace `readTask` (#10432)
  chore: release next
  fix(tip): fix rendering tied to named-slot content (#10470)
  ci: compile estimate totals per milestone (#10442)
  chore: release next
  fix(modal): fix rendering tied to named-slot content (#10469)
  chore: release next
  fix(shell-center-row): fix rendering tied to named-slot content (#10451)
  fix(inline-editable): fix rendering tied to default slot content (#10456)
  fix(input, input-number, input-text): should not set slotted actions to be disabled (#10458)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issues with changes that don't modify src or test files. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants