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(card-group): support RTL borders & padding #11760

Merged

Conversation

jkaeser
Copy link
Member

@jkaeser jkaeser commented May 1, 2024

Related Ticket(s)

Closes #11755

Description

We programmatically set borders & padding for cards within <dds-card-group> elements, and we've only been accounting for LTR writing directions. This PR uses logical properties so these borders and paddings look correct in RTL documents.

While working on this I noticed an issue where certain tracked elements could be undefined, causing an error that halts execution. Ultimately this resulted in the "empty" elements we generate for certain card groups to fail to be created, and the dark gray background appearing when it shouldn't. I fixed this with simple optional accessors.

Testing

Verify the borders and gaps between Cards appear correctly in the Card Group story in the RTL deploy preview. Compare to the Card Group in the default deploy preview if necessary.

Specifically check that no background is visible in the last row's empty space when using the "Outlined cards (1px border)" Grid mode option.

Changelog

Changed

  • <dds-card-group> children will receive correct border & padding values in RTL writing mode documents.
  • Prevents undefined elements from halting code execution and causing gray background to show when it shouldn't.

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@jkaeser jkaeser added package: styles Work necessary for the Carbon for IBM.com styles package RTL Assigned to Arabic Right to Left work package: web components Work necessary for the IBM.com Library web components package owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants labels May 1, 2024
@jkaeser jkaeser marked this pull request as ready for review May 1, 2024 19:41
@jkaeser jkaeser requested a review from a team as a code owner May 1, 2024 19:41
@jkaeser jkaeser closed this May 1, 2024
@jkaeser jkaeser reopened this May 1, 2024
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented May 1, 2024

@andy-blum andy-blum added the Needs design approval PRs on feature requests and new components have to get design approval before merge. label May 2, 2024
Copy link
Contributor

@m4olivei m4olivei left a comment

Choose a reason for hiding this comment

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

Looks good! Nice work.

@jkaeser jkaeser requested review from RichKummer and oliviaflory May 3, 2024 13:56
@oliviaflory
Copy link
Contributor

@jkaeser cc @andy-blum

The text looks great!

The only thing that looks off is the arrow icons.

  1. My understanding is directional icons would be mirrored (see examples attached)
  2. The arrow icon in the card group has a 32px right padding on it which makes it not align with the text as expected, it's floating in the card

Extra space to the right of the arrow or CTA
Screenshot 2024-05-03 at 9 58 17 AM
Screenshot 2024-05-03 at 10 02 06 AM

Expectation
Screenshot 2024-05-03 at 10 15 49 AM

@kennylam kennylam added the Ready to merge Label for the pull requests that are ready to merge label May 3, 2024
@kodiakhq kodiakhq bot merged commit d2a0328 into carbon-design-system:v1 May 3, 2024
23 of 26 checks passed
@jkaeser
Copy link
Member Author

jkaeser commented May 3, 2024

@oliviaflory Good catch! This got merged before I could address it, but that's okay. Matt has fixed the arrow direction problem in #11748, which should be merged soon. I believe that only leaves the extra spacing to the right to fix. I'll open up an issue for it.

Thanks for taking a look at this one!

@jkaeser
Copy link
Member Author

jkaeser commented May 3, 2024

Created #11762 for the spacing issue.

@jkaeser jkaeser deleted the fix/card-rtl-borders-padding branch May 3, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 eyes needed Needs design approval PRs on feature requests and new components have to get design approval before merge. owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants package: styles Work necessary for the Carbon for IBM.com styles package package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge RTL Assigned to Arabic Right to Left work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants