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

[Selectable Components: Structured List - Style] Website Doc #3965

Merged

Conversation

Kritvi-bhatia17
Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 commented Mar 11, 2024

Closes #15775

For ref: Figma file


Changelog

New

Color & Interactive states section

  • Added an image for both Transparent and Background structured list (enabled states)
  • Added the color specs for structured list with background as 'optional'

Structure section

  • Add Default structure & Selectable structure sections with description and images each
  • Add a new section(Size section) with description and images each

Changed

Color, Interactive states & Structure section

  • Updated the table have the correct color tokens

Copy link

vercel bot commented Mar 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
carbondesignsystem ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2024 7:33am

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 👍 ✅

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.

Looking good Kritvi! Just some comments below:

Structure section

  • Since you are showing the hang and flush structured list specs in the image, the table above it also needs to have some information for the flush list specs. The table currently only has information for the hang structured list specs.
  • I would also add specs to the table for the Icon left and right padding.
  • In the Selectable image, the icon needs to be called out in magenta as being 16px in size.

src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
src/pages/components/structured-list/style.mdx Outdated Show resolved Hide resolved
Kritvi-bhatia17

This comment was marked as duplicate.

This reverts commit eb6a4f7.
@laurenmrice
Copy link
Member

Looks good Kritvi! I think after these small image bugs, it should be good to go!

I would check the bottom and top spacing in these areas in red in the following images (for both background and transparent images) to make sure they are the same size and that the component is centered in the center of the space. I can't tell if its the tab component in gatsby that is causing this, or if its in the actual image file.

Screenshot 2024-03-21 at 8 26 39 AM Screenshot 2024-03-21 at 8 30 57 AM

This image just needs to bring this top divider in, its too long.
Screenshot 2024-03-21 at 8 33 12 AM

@Kritvi-bhatia17
Copy link
Contributor Author

Looks good Kritvi! I think after these small image bugs, it should be good to go!

I would check the bottom and top spacing in these areas in red in the following images (for both background and transparent images) to make sure they are the same size and that the component is centered in the center of the space. I can't tell if its the tab component in gatsby that is causing this, or if its in the actual image file.

Screenshot 2024-03-21 at 8 26 39 AM Screenshot 2024-03-21 at 8 30 57 AM
This image just needs to bring this top divider in, its too long. Screenshot 2024-03-21 at 8 33 12 AM

Hi Lauren!
Oh damn, thank you for checking. Here are a couple of points:

  • The preview link isn't showing the latest update for some reason.
  • While the images have the corrected spacing, there seems to be an issue with the code introducing a tab above it, causing unequal spacing top and bottom. I'm not sure if it's a code issue, but the images in Figma have the correct spacing. I think I might need to connect with one of the developers to address this.
  • Regarding the last image you pointed out, if you check under files changed, it appears to be correct there, but it's not reflecting correctly on the preview link provided.

Thanks again!

@Kritvi-bhatia17
Copy link
Contributor Author

Looks good Kritvi! I think after these small image bugs, it should be good to go!

I would check the bottom and top spacing in these areas in red in the following images (for both background and transparent images) to make sure they are the same size and that the component is centered in the center of the space. I can't tell if its the tab component in gatsby that is causing this, or if its in the actual image file.

Screenshot 2024-03-21 at 8 26 39 AM Screenshot 2024-03-21 at 8 30 57 AM
This image just needs to bring this top divider in, its too long. Screenshot 2024-03-21 at 8 33 12 AM

Hi @laurenmrice! I spoke with @tay1orjones to clarify a couple of points:

  1. Regarding the padding issue, I've raised an issue after discussing with @tay1orjones. There seems to be unequal padding set in the code, and we might need to discuss it with the team.
  2. The preview link has been updated, and it now displays the latest images.

The padding issue might take some time to resolve. In the meantime, I'll go ahead and merge this PR. Just wanted to keep you in the loop!

@Kritvi-bhatia17 Kritvi-bhatia17 merged commit 4af56d8 into carbon-design-system:main Apr 28, 2024
7 checks passed
@Kritvi-bhatia17 Kritvi-bhatia17 deleted the SL_style_#15775 branch September 23, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Selectable Components: Structured List - Style] Website Doc
5 participants