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(StructuredList): add prop for condensed list #8446

Merged
merged 5 commits into from
May 14, 2021

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Apr 16, 2021

Closes #5736

New

  • adds boolean for condensed list support (styles were already supported in vanilla)

@jnm2377 jnm2377 requested a review from a team April 16, 2021 21:52
@jnm2377 jnm2377 requested a review from a team as a code owner April 16, 2021 21:52
@jnm2377 jnm2377 requested review from kingtraceyj, joshblack and dakahn and removed request for a team April 16, 2021 21:52
@netlify
Copy link

netlify bot commented Apr 16, 2021

Deploy preview for carbon-elements ready!

Built with commit 486105d

https://deploy-preview-8446--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Apr 16, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 486105d

https://deploy-preview-8446--carbon-components-react.netlify.app

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

@aagonzales
Copy link
Member

@jnm2377 Is there a design spec somewhere that you're referencing? Is the removal of the padding-left intentional?

@jnm2377
Copy link
Contributor Author

jnm2377 commented Apr 19, 2021

@aagonzales I didn't do anything with the styles, these already exist. I simply added a prop to be able to toggle the styles in the react component. As far as the padding, it seems like that was removed several years ago bc of this issue: #693

@aagonzales
Copy link
Member

aagonzales commented Apr 19, 2021

"already supported in vanilla" I think it's that vanilla was never updated. This condensed style matches the old v9 styling of structure list. What you see in the "simple" story is the intended design. I don't think there was ever supposed to be a condensed version with the v10 update. I'll take this to the greater design team and ask them.

For that context, we don't mention it in our website docs or include it as a part of the Sketch kit. Just trying to figure out where this ask is actually coming from.

@jnm2377
Copy link
Contributor Author

jnm2377 commented Apr 19, 2021

@aagonzales I guess on the dev side we assumed this issue was ok to work on bc no designers ever declined the enhancement in the original issue when Akira asked for input (this was about a year ago). Lmk what design decides.
Screen Shot 2021-04-19 at 12 48 00 PM

@aagonzales
Copy link
Member

aagonzales commented Apr 19, 2021

(in a very joking tone) we were a little distracted in March 2020 so I think this one just fell through the cracks.

We talked about it in our design sync today, we think a condensed version is ok to have but this style is is flush as well as condensed and those would be two separate props. I'm going to mock-up a quick spec to demonstrate what I mean.

@aagonzales
Copy link
Member

Ok so adding a condensed list is one issue and there's a second one about adding a prop for flush content instead of hanging content.

I know this PR is just about enabling the prop but I think its an old one and was never properly updated with v10 styling. I have talked to the design team and we're ok with having these 4 variants. Or to start simply adding in the indented padding to the left of the first column on the condensed list to match the padding of the default list.

image

Copy link
Member

@kingtraceyj kingtraceyj left a comment

Choose a reason for hiding this comment

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

For condensed, padding between columns should be 16px.

Current (8px):
image

Proposed (16px):
image

@jnm2377
Copy link
Contributor Author

jnm2377 commented Apr 29, 2021

@kingtraceyj @aagonzales updated to include both options via props. There's a playground story where you can toggle the diff styles to test, and also added knobs to the selection story to view on there.

Copy link
Member

@kingtraceyj kingtraceyj left a comment

Choose a reason for hiding this comment

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

Condensed, Flush should have 16px between columns
Current:
image

Proposed:
image

Hang alignment, default spacing should have 16px spacing
current is 32px (there's 16px before and after):
image

Proposed:
image

@jnm2377 jnm2377 requested a review from kingtraceyj May 14, 2021 00:00
Copy link
Member

@kingtraceyj kingtraceyj left a comment

Choose a reason for hiding this comment

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

good to go 💥 thanks josefina! looks great.

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.

feat(structured-list): Add React component support for the condensed structured list styles
6 participants