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(RTL): swapped left/right spacing with css logical properties #2158

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

ArtBlue
Copy link
Contributor

@ArtBlue ArtBlue commented Sep 26, 2023

Fixes #1885

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

This issue is for refactoring margin-left/right for RTL into logical properties of margin-inline-start/end.

Notes

Here are the details of the work with note. Some of the RTL blocks cannot be removed as they include RTL updates that cannot be accomplished with logical properties. For example, transform: rotate(180deg). Also, some of the files need more major refactored as they include a mix of margin-left, margin-right, absolute positioning with left and right. In other instances, the RTL mode is merely a fix to address some edge case that the LTR mode doesn’t need to address.

The following modules have some notes on exceptions of refactoring:

MODULES NOTES
Breadcrumbs NOT Needed
Button I’m very confused about this one and not sure how to proceed. This needs many more stories, specifically with icons inside in default mode and RTL.
Carousel DONE
Combobox DONE . Some of this looks to require an overhaul. We’re using margins for icons that are absolutely positioned inside the control. Create an issue for the other adjustments
CTA Button No changes needed; No margins
Details DONE
Field DONE
Filter Menu Button DONE . filter-menu-button__checkbox has an RTL block margin adjustment, but no corresponding margin exists as an LTR default
Floating Label NOT Needed
Inline Notice DONE . I fixed the RTL target to match the margin of the original parent instead of the icon inside. There was a subtle bug with the notice where the right margin (in header) was persisting and creating additional space on the right (unnecessary right margin) in RTL.
Lightbox Dialog DONE
Listbox DONE
Listbox Button DONE
Dropdown Mixin NOT needed
Page Notice DONE (partial). .page-notice__footer needs more substantial work to align/simplify things.
Pagination DONE
Progress Stepper DONE
Section Notice DONE (partial). .section-notice__footer and section-notice__cta need more substantial work to align/simplify things.
Section Title DONE . This had a whole bunch of margin: 0; and margin: auto that should be looked into in the future. On the surface, they appear to be doing nothing.
Segmented Button DONE
Select Not Needed. RTL block spacing looks to be doing some other thing than reversing LTR.
Snackbar Dialog DONE
Tabs DONE
Textbox DONE . We need many more stories for this including icons for both start and end.
Toast Dialog DONE
Video DONE

Screenshots

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

Copy link
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

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

LGTM.

dist/button/button.css Show resolved Hide resolved
@ArtBlue ArtBlue changed the base branch from 17.0.0 to 16.8.0 September 28, 2023 20:20
@ArtBlue ArtBlue merged commit 184fdb4 into 16.8.0 Sep 29, 2023
@ArtBlue ArtBlue deleted the 1885-use-css-logical-properties branch September 29, 2023 15:35
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.

4 participants