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(pattern): fixed gutter between cards using gridCondensed property #1034

Merged

Conversation

mkothur
Copy link
Contributor

@mkothur mkothur commented Jan 14, 2020

Related Ticket(s)

Related to ticket #976

Description

Added the grid condensed property in cardSection pattern which will help to create spacing between cards. And restructure the JSX little bit to match the CSS changes.

And also added margin in logo_grid CSS to look nicer.

Changelog

Changed

CardSection - js and scss
logGrid - scss

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jan 14, 2020

> *:not(:last-child) {
margin-bottom: 0;
margin-top: $carbon--grid-gutter--condensed / 2;
margin-bottom: $carbon--grid-gutter--condensed / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I saw carbon-design-system/carbon#4960 got merged in. These lines of code may not be necessary anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean we could remove these props and just have ${prefix}--grid--condensed this in JSX right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we do need margin-bottom: $carbon--grid-gutter--condensed / 2 this prop in that I guess. Because when I remove both of them it looks like this.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like they might not have releasee it yet for some reason. I thought they were doing nightly builds. I guess we can wait a little longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

@photodow
Copy link
Contributor

photodow commented Jan 14, 2020

This is looking a lot cleaner! 🎉

With that said there are some minor grid alignment issues we need to resolve.

It doesn't have a max-width because it looks like we didn't include the bx--make-container mixin.
image

Unless it's supposed to be full width, in that case it's still slightly off.
image


I think the learn template maxes out its width out, but in general should we plan for when the adopter uses the full-width grid modifier? @jeffchew @annawen1 @kennylam @raphaelamadeu

@mkothur mkothur force-pushed the bug/976-cardSec-condensedGrid branch from 6f5dd8f to f8771d5 Compare January 15, 2020 15:23
@mkothur mkothur force-pushed the bug/976-cardSec-condensedGrid branch from 2b925a0 to 744d855 Compare January 17, 2020 14:11
@@ -77,7 +77,7 @@ const cardsGroup = {
},
{
image: {
defaultImage: 'http://picsum.photos/id/1003/1056/480',
defaultImage: 'https://picsum.photos/id/1018/1056/480',
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't add a comment, but on line L#67 above. The image isn't rendering. I added https, and it seemed to work for me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah @photodow: the s is missing from the image URL because of the feature link commit from @carthick
image
Let me update the PR.

Copy link
Contributor

@photodow photodow left a comment

Choose a reason for hiding this comment

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

Just have the image issue, otherwise the grid looks so much better here. nice work!

@mkothur mkothur force-pushed the bug/976-cardSec-condensedGrid branch from 744d855 to 0127eac Compare January 17, 2020 18:41
@jeffchew jeffchew merged commit 7edc499 into carbon-design-system:master Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants