-
Notifications
You must be signed in to change notification settings - Fork 160
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(content-group): v2 style updates #11007
feat(content-group): v2 style updates #11007
Conversation
… into feat/content-group-v2
… into feat/content-group-v2
Deploy preview created for package Built with commit: 6a824fa5b862afa209b9af845785349c5deaf362 |
Deploy preview created for package Built with commit: 6a824fa5b862afa209b9af845785349c5deaf362 |
Deploy preview created for package Built with commit: 6a824fa5b862afa209b9af845785349c5deaf362 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @sangeethababu9223 ! There seems to be a slight difference between the v1 and v2 examples when I add the children elements. In v2 the content items extend beyond the width of the content group, this might have more to do with the content item than the content group? But I am not quite sure. I know @kennylam is still working on the content item We do want the content group to be able to accept 8 or 12 column children, similar the content section updates, so I am OK with merging this AS-IS for now and returning to it once we have more of the content components figured out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sangeethababu9223 , two other things to add to Olivia's comment above:
- When nesting
content items
intocontent group
, the top and bottom margins should merge together. But right now, it looks like the spacing values stack instead. @kennylam and @IgnacioBecerra were solving a similar problem recently, so they may have some insight.
You can see in the following screenshots that the margin spacing is stacking rather than merging together:
- The last update is that the last nested piece of content inside
content group
should automatically remove the bottom spacing (or merge it) so that there is48px
spacing between the bottom of the last item and theCTA
.
You can see in the following screenshot that the spacing stacks, creating more space at the end than intended.
… into feat/content-group-v2
… into feat/content-group-v2
Hey @RichKummer , @oliviaflory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @sangeethababu9223 !
94e9d94
into
carbon-design-system:feat/carbon-for-ibm-dotcom-v2
Related Ticket(s)
Closes #10753
Description
Style issues with Content Group Component in V2
Changelog
Changed