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(structured-list): add selected background border #13384

Merged

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Mar 21, 2023

Closes #5232
Closes #13383

Changelog

Adds selected background state for structured list. Noticed a bug with the border while adding this and created a new issue.

New

  • add selected background state

Testing / Reviewing

Check that selected state is correct
https://deploy-preview-13384--carbon-components-react.netlify.app/?path=/story/components-structuredlist--selection

Thanks @francinelucca for the cross-browser fix! 🥳

@netlify
Copy link

netlify bot commented Mar 21, 2023

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit c8fe4af
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/643031ec5c8fe40008772e23
😎 Deploy Preview https://deploy-preview-13384--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 21, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c8fe4af
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/643031edab08be0008d183e0
😎 Deploy Preview https://deploy-preview-13384--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@alisonjoseph alisonjoseph marked this pull request as draft March 22, 2023 18:38
@alisonjoseph alisonjoseph marked this pull request as ready for review March 22, 2023 18:43
Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

LGTM!

@tw15egan
Copy link
Member

Any idea what this little border variance is? It is on main as well, so it seems like an existing bug

2023-03-22 15 37 32

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Mar 22, 2023

Yea I opened an issue for that yesterday, I think it has to do with the radio button not being inside a td, but didn't spend too much time on it. The selected state does make it more noticeable #13383 @tw15egan

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

@alisonjoseph

  • The hover and selected state backgrounds should overlap or hide the rule dividers if that is possible.
  • The hover state background color should be $layer-hover. I would just double-check because it seems lighter and might currently be $background-layer.

Artboard

@alisonjoseph
Copy link
Member Author

Re-requesting reviews @tw15egan @laurenmrice @francinelucca

This PR now should have correct hover and border colors, it should hide the border when on hover/selected and it should fix the border bug from #13383

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

LGTM

@tw15egan
Copy link
Member

tw15egan commented Apr 4, 2023

@alisonjoseph the fix looks great! But I'm able to click and change the background on the default StructuredList now as well

@alisonjoseph
Copy link
Member Author

@tw15egan good catch! Should be fixed now 😅

Copy link
Member

@laurenmrice laurenmrice 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 to me! ✅

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@kodiakhq kodiakhq bot merged commit f2725e0 into carbon-design-system:main Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants