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(square-card): add new square card component #1151

Merged
merged 15 commits into from
Sep 27, 2021
Merged

feat(square-card): add new square card component #1151

merged 15 commits into from
Sep 27, 2021

Conversation

aledavila
Copy link
Contributor

@aledavila aledavila commented Jul 26, 2021

Closes #1061

Changelog

New

  • Added Square Card component
  • To test make check Square Card page appears

@aledavila aledavila requested review from a team, vpicone and sstrubberg and removed request for a team July 26, 2021 17:04
@vercel
Copy link

vercel bot commented Jul 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-design-system/gatsby-theme-carbon/FcYacPcBejvjK9v2YbWGbLRB2WKo
✅ Preview: https://gatsby-theme-carbon-git-fork-aledav-9bb2b5-carbon-design-system.vercel.app

@aledavila aledavila changed the title DO NOT MERGE - feat(square-card): add new square card component feat(square-card): add new square card component Aug 2, 2021
@aledavila
Copy link
Contributor Author

@jeanservaas @vpicone @sstrubberg Its ready for review now :)

@sstrubberg
Copy link
Member

image

Might be a good idea to drop to a small type token at large screens.

@aledavila
Copy link
Contributor Author

aledavila commented Aug 5, 2021

@sstrubberg Good point.

So @jeanservaas between large and xlg screens I've made it instead productive-heading-03 and productive-heading-02 for the large and small titles. Cause then we have the above happen. Could you give input on this please. otherwise its still productive-heading-04 and productive-heading-03 for the rest of the screens since they fit fine. Should the pictogram also change size or can it remain at the default size.

@jeanservaas
Copy link
Contributor

jeanservaas commented Aug 5, 2021

@sstrubberg @aledavila Looks amazing!!!

I think what you guys have done with the adaptive approach is good for the two square cards that have sentences. Wondering if for this "short title" one, if it would work to have it stay at $productive-heading-04 at all breakpoints instead of jumping down to $productive-heading-03.

image

Also can we just shorten the small bottom line to say (Optional text)?

I don't really want people writing a lot here, it's more appropriate for say, a date or a category name than a sentence.

image

One last thing, it looks like you're using 24px type at the largest size for those top three cards... $productive-heading-04 is actually 28px type, which is what those should be... Are you using the $expressive-paragraph-01 token for those or something?

Notice, the sentence size is not the same as the heading size above it.

image

Copy link
Contributor

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

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

See comment below

@aledavila
Copy link
Contributor Author

@jeanservaas It appears the the headings are 32px. I’m using productive-04. The computed font-size for the cards says it’s 28px. I think the example headings is what is incorrect are those supposed to be 28px I can submit a change for that.

Also to answer your question one thing we can do is add a prop that if a user adds that prop it’ll be productive-04 on all screen sizes. Like a “shortTitle” prop for example or instead do default productive-04 with a prop “sentenceTitle” that would make it productive-03. That would be the easiest solution. Otherwise you’d have to make a function to consider text length but the cleanest would be the prop. What are your thoughts on that ?

@alisonjoseph
Copy link
Member

@aledavila @jeanservaas can we get this finished up next sprint? seems like its close?

@jeanservaas
Copy link
Contributor

Hey Ale, we either need to drop these Optional type moments at smaller sizes, break the line sooner (i.e there'd be about 24px of space between the icon and the end of the type), or cut the extra type, we can't allow this crunch up to happen at small type sizes.

image

Also, for this adaptive scenario, the space after the type is fine, because these are two different type sizes

image

but beyond the large breakpoint, when the type sizes become the same, there would be no extra space between the heading and the body copy

image

So it would look like this if we go down a type size

image

All else looks good. @aledavila can you ping me when you get this done and I'll give you a review really fast. Sorry.

Copy link
Contributor

@jeanservaas jeanservaas 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!

Copy link
Collaborator

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

Looks great! Awesome job. Thanks for making those changes.

@vpicone vpicone merged commit 8438c05 into carbon-design-system:main Sep 27, 2021
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.

Enhancement[card]: add 1:1 card to component library
5 participants