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

fix: deprecate transform logical plugin for vanilla values #2437

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

castastrophe
Copy link
Collaborator

@castastrophe castastrophe commented Jan 16, 2024

Description

Transform logical is currently converting values to rtl versions when a special keyword is provided. This is useful however, because it involves adding a non-(browser-)specified keyword to transforms, causes more confusion when onboarding than it helps.

This PR proposes removing the transform-logical utility in favor of hardcoding the values for better clarity for contributors.

How and where has this been tested?

Should result in no rtl regressions for the following components:

  • accordion - indicator chevron direction swaps in rtl
  • actionbutton - hold icon appears on opposite side in rtl
  • assetlist - item indicator appears on opposite side in rtl
  • breadcrumb - separator faces opposite direction in rtl
  • calendar - prev & next month chevrons swap in rtl
  • pagination - prev & next month chevrons swap in rtl
  • slider - ramp increases in opposite direction for rtl
  • table - table disclosure rotates in opposite direction for rtl
  • treeview - open item indicator faces opposite direction for rtl

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch from db55136 to 16b2453 Compare January 16, 2024 21:16
@castastrophe castastrophe added run_vrt For use on PRs looking to kick off VRT ready-for-review labels Jan 16, 2024
Copy link
Contributor

github-actions bot commented Jan 16, 2024

File metrics

Summary

Total size: 3.91 MB*
Total change (Δ): ⬇ 1.31 KB (-0.03%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package Size Δ
accordion 17.81 KB ⬇ 0.66 KB
actionbutton 40.97 KB ⬆ 0.10 KB
assetlist 7.20 KB ⬆ 0.08 KB
breadcrumb 16.37 KB ⬇ 0.05 KB
calendar 20.63 KB ⬆ 0.02 KB
pagination 2.50 KB ⬆ 0.04 KB
slider 32.15 KB ⬆ 0.09 KB
table 46.42 KB ⬇ 0.08 KB
treeview 17.21 KB ⬇ 0.15 KB
Details

accordion

File Head Base Δ
index-base.css 17.81 KB 18.45 KB ⬇ 0.66 KB (-3.49%)
index-vars.css 17.81 KB 18.45 KB ⬇ 0.66 KB (-3.49%)
index.css 17.81 KB 18.45 KB ⬇ 0.66 KB (-3.49%)
mods.json 1.79 KB 1.79 KB -

actionbutton

File Head Base Δ
index-base.css 29.90 KB 29.80 KB ⬆ 0.10 KB (0.31%)
index-theme.css 11.65 KB 11.65 KB -
index-vars.css 40.97 KB 40.88 KB ⬆ 0.10 KB (0.23%)
index.css 40.97 KB 40.88 KB ⬆ 0.10 KB (0.23%)
mods.json 2.63 KB 2.63 KB -
themes/express.css 8.92 KB 8.92 KB -
themes/spectrum.css 9.20 KB 9.20 KB -

assetlist

File Head Base Δ
index-base.css 7.20 KB 7.13 KB ⬆ 0.08 KB (1.06%)
index-vars.css 7.20 KB 7.13 KB ⬆ 0.08 KB (1.06%)
index.css 7.20 KB 7.13 KB ⬆ 0.08 KB (1.06%)
mods.json 0.90 KB 0.90 KB -

breadcrumb

File Head Base Δ
index-base.css 16.37 KB 16.42 KB ⬇ 0.05 KB (-0.32%)
index-vars.css 16.37 KB 16.42 KB ⬇ 0.05 KB (-0.32%)
index.css 16.37 KB 16.42 KB ⬇ 0.05 KB (-0.32%)
mods.json 2.60 KB 2.60 KB -

calendar

File Head Base Δ
index-base.css 20.63 KB 20.62 KB ⬆ 0.02 KB (0.08%)
index-vars.css 20.63 KB 20.62 KB ⬆ 0.02 KB (0.08%)
index.css 20.63 KB 20.62 KB ⬆ 0.02 KB (0.08%)
mods.json 1.67 KB 1.67 KB -

pagination

File Head Base Δ
index-base.css 2.50 KB 2.46 KB ⬆ 0.04 KB (1.39%)
index-vars.css 2.50 KB 2.46 KB ⬆ 0.04 KB (1.39%)
index.css 2.50 KB 2.46 KB ⬆ 0.04 KB (1.39%)
mods.json 0.29 KB 0.29 KB -

slider

File Head Base Δ
index-base.css 29.84 KB 29.75 KB ⬆ 0.09 KB (0.29%)
index-theme.css 2.89 KB 2.89 KB -
index-vars.css 32.15 KB 32.06 KB ⬆ 0.09 KB (0.27%)
index.css 32.15 KB 32.06 KB ⬆ 0.09 KB (0.27%)
mods.json 2.19 KB 2.19 KB -
themes/express.css 1.75 KB 1.75 KB -
themes/spectrum.css 1.74 KB 1.74 KB -

table

File Head Base Δ
index-base.css 46.42 KB 46.50 KB ⬇ 0.08 KB (-0.17%)
index-vars.css 46.42 KB 46.50 KB ⬇ 0.08 KB (-0.17%)
index.css 46.42 KB 46.50 KB ⬇ 0.08 KB (-0.17%)
mods.json 4.01 KB 4.01 KB -

treeview

File Head Base Δ
index-base.css 17.21 KB 17.36 KB ⬇ 0.15 KB (-0.86%)
index-vars.css 17.21 KB 17.36 KB ⬇ 0.15 KB (-0.86%)
index.css 17.21 KB 17.36 KB ⬇ 0.15 KB (-0.86%)
mods.json 1.64 KB 1.64 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Jan 16, 2024

🚀 Deployed on https://pr-2437--spectrum-css.netlify.app

Copy link
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

Just the one comment for discussion.

components/accordion/index.css Outdated Show resolved Hide resolved
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch from a264d53 to 27a90a7 Compare January 17, 2024 21:54
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch 2 times, most recently from a344bf1 to 5f13e56 Compare January 18, 2024 19:05
Copy link
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

I went through the validation links, and the ones that I've left unchecked seem to have chevrons that are either pointing the incorrect direction in the LTR state, or in the RTL state, or both.

I think maybe we're missing the rotate() portion of the transform on some of those?

@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch from 5f13e56 to 150a8ac Compare January 18, 2024 22:12
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch 2 times, most recently from 56309e2 to 11b2b33 Compare January 22, 2024 16:23
@castastrophe castastrophe added the high priority An important PR or issue requiring immediate attention label Jan 22, 2024
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch from 11b2b33 to 636b9e0 Compare January 22, 2024 17:53
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch 2 times, most recently from 71197ec to e841a6b Compare January 22, 2024 18:09
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch 3 times, most recently from addd693 to 24bfc05 Compare January 22, 2024 20:12
@castastrophe castastrophe force-pushed the fix-deprecate-transform-logical branch from 24bfc05 to a7181f1 Compare January 22, 2024 20:13
@castastrophe castastrophe enabled auto-merge (squash) January 22, 2024 20:21
@castastrophe castastrophe merged commit be2974e into main Jan 22, 2024
17 of 21 checks passed
@castastrophe castastrophe deleted the fix-deprecate-transform-logical branch January 22, 2024 20:31
@github-actions github-actions bot mentioned this pull request Apr 19, 2024
@github-actions github-actions bot mentioned this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority An important PR or issue requiring immediate attention ready-for-review run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants