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

feat: added dividers accross multiple states for AI select #15952

Conversation

riddhybansal
Copy link
Contributor

Closes #15860

Added dividers accross multiple states for AI select

Changelog

New

  • Added dividers accross multiple states for AI select

Testing / Reviewing

  • Go to the storybook and test the divider by adding warn/invalid to AI select

@riddhybansal riddhybansal requested a review from a team as a code owner March 13, 2024 13:09
Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit fd2bc32
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/660d841bb976a20008b65156
😎 Deploy Preview https://deploy-preview-15952--v11-carbon-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 configuration.

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

Hey @riddhybansal LGTM 🚀

Just on thing in the Select component
Screenshot 2024-03-13 at 11 00 01

packages/styles/scss/components/select/_select.scss Outdated Show resolved Hide resolved
tay1orjones

This comment was marked as resolved.

@tay1orjones
Copy link
Member

tay1orjones commented Mar 13, 2024

A few other questions I just thought of:

  • For the px values, could we derive those from the spacing tokens in certain situations? Like, margin of 9 px is actually $spacing-03 + 1, the 1 being the divider width
    image

  • Does this need to be supported for the other size options? If so, could we add a control to be able to see how it works with the other sizes?

@tay1orjones
Copy link
Member

  • Does this need to be supported for the other size options?

@alina-jacob could you help answer this? The spec only refers to horizontal spacing, so I think the existing solution would work for all sizes if the values should be the same on every size.

@alina-jacob
Copy link

  • Does this need to be supported for the other size options?

@alina-jacob could you help answer this? The spec only refers to horizontal spacing, so I think the existing solution would work for all sizes if the values should be the same on every size.

Hi @tay1orjones you're right, the spacing stays the same across large, medium and small sizes.

@alisonjoseph
Copy link
Member

Confirmed styles work across all sizes

Screenshot 2024-03-20 at 10 58 41 AM


.#{$prefix}--select--slug .#{$prefix}--slug::before {
display: none;
inset-inline-start: convert.to-rem(-9px);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inset-inline-start: convert.to-rem(-9px);
inset-inline-start: calc(-#{$spacing-03} - 1px);

In the spirit of @tay1orjones PR around magic numbers, would we want to update these values to be the padding value plus the divider size?

Copy link

@alina-jacob alina-jacob 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 Riddhi! 🚀
thanks!

@tay1orjones tay1orjones added this pull request to the merge queue Apr 3, 2024
Merged via the queue into carbon-design-system:main with commit 5bd4f4a Apr 3, 2024
20 checks passed
preetibansalui pushed a commit to tay1orjones/carbon that referenced this pull request Apr 24, 2024
…sign-system#15952)

* feat: added dividers accross multiple states for AI select

* fix: some styles

* fix: some styles

* fix: styling with magic numbers

---------

Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>
Co-authored-by: TJ Egan <tw15egan@gmail.com>
Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Introduce Divider across multiple states for AI Select
7 participants