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

Update styleguide colors & typography #2236

Merged
merged 7 commits into from
Mar 19, 2020
Merged

Update styleguide colors & typography #2236

merged 7 commits into from
Mar 19, 2020

Conversation

jimbo
Copy link
Contributor

@jimbo jimbo commented Mar 10, 2020

Description

This PR introduces the first aspects of new design to the Venia Styleguide. It updates colors & typography, and uses them to update the button component's presentation.

This PR also renames the footer component to button group, since it's not really a footer, and updates it to reflect guidance from the new design.

Related Issue

  • PWA-433
  • PWA-435

Acceptance

@soumya-ashok
@schensley

Verification Stakeholders

Specification

Verification Steps

  1. Visit /page/color and verify the colors are correct
  2. Visit /page/typography and verify the fonts are correct
  3. Visit /page/button and verify the appearance and guidelines are correct
  4. Visit /page/button-group and verify the appearance and guidelines are correct

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@jimbo jimbo added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Mar 10, 2020
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Mar 10, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Mar 10, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-433.

Generated by 🚫 dangerJS against 3b4e7a6

@soumya-ashok
Copy link

@jimbo - Looks good overall. A few thoughts -

  1. Could we indicate which the brand color is and which ones are derivatives?
  2. We originally had 3 weights for the sans serif which may be helpful as we scale the design since we haven't hit all the use cases yet. 2 might be limiting.
  3. I think we could do with just one variant on the serif since it is used only for headings
  4. I'm not sure we're using italics anywhere yet in the body, but probably good to have the option open?
  5. Button label might need to have a heavier font weight. I'm unsure about the overflow bit especially if the button label is in a different language that might cause issues? How would someone view the overflow text?

@schensley Could you also take a look if possible?

@jimbo
Copy link
Contributor Author

jimbo commented Mar 16, 2020

1. Could we indicate which the brand color is and which ones are derivatives?

Yes. I've pushed this change.

2. We originally had 3 weights for the sans serif which may be helpful as we scale the design since we haven't hit all the use cases yet. 2 might be limiting.

We currently use 400 and 600 for everything, though in this PR we're adding 700 for buttons (and potentially other things). Fonts are an expensive resource and each weight adds a lot to the initial load, so ideally we would draw a line here and try not to use more than these three, especially since we've already accomplished a nearly full design with what we have.

3. I think we could do with just one variant on the serif since it is used only for headings

I agree. I've pushed this change.

4. I'm not sure we're using italics anywhere yet in the body, but probably good to have the option open?

Again, adding another style would increase the load a fair bit—especially if we were to do so for all three weights. Also, the browser can simulate italics for a font with a fair degree of accuracy without loading any actual new glyphs. Could we rely on that, given that we will hardly be using italics (if we do at all)?

5. Button label might need to have a heavier font weight. I'm unsure about the overflow bit especially if the button label is in a different language that might cause issues? How would someone view the overflow text?

I agree. I've pushed a change adding the 700 weight for buttons. We can change the buttons to overflow to a second line, if you like; the new button style can probably handle it better.

@soumya-ashok
Copy link

soumya-ashok commented Mar 17, 2020

1. Could we indicate which the brand color is and which ones are derivatives?

Yes. I've pushed this change.

Looks good

  1. We originally had 3 weights for the sans serif which may be helpful as we scale the design since we haven't hit all the use cases yet. 2 might be limiting.

We currently use 400 and 600 for everything, though in this PR we're adding 700 for buttons (and potentially other things). Fonts are an expensive resource and each weight adds a lot to the initial load, so ideally we would draw a line here and try not to use more than these three, especially since we've already accomplished a nearly full design with what we have.

Yes, not planning to use more than 3. The buttons look better now with 700.

  1. I think we could do with just one variant on the serif since it is used only for headings

I agree. I've pushed this change.

Looks good

  1. I'm not sure we're using italics anywhere yet in the body, but probably good to have the option open?

Again, adding another style would increase the load a fair bit—especially if we were to do so for all three weights. Also, the browser can simulate italics for a font with a fair degree of accuracy without loading any actual new glyphs. Could we rely on that, given that we will hardly be using italics (if we do at all)?

Yes.

  1. Button label might need to have a heavier font weight. I'm unsure about the overflow bit especially if the button label is in a different language that might cause issues? How would someone view the overflow text?

I agree. I've pushed a change adding the 700 weight for buttons. We can change the buttons to overflow to a second line, if you like; the new button style can probably handle it better.

I wonder if the button should become more of a square with rounded corners if the label were to wrap? The shape of the fully rounded button seems compromised when this happens.

Another general question - I see that there are no visuals for a few sections on the button and typography pages. Assume the gray placeholder boxes would be replaced with these at a later time? Let me know if you need anything from design for this.

@jimbo
Copy link
Contributor Author

jimbo commented Mar 18, 2020

I wonder if the button should become more of a square with rounded corners if the label were to wrap? The shape of the fully rounded button seems compromised when this happens.

Oh, I meant to fix that. CSS doesn't tell us when the contents have overflowed (wrapped, in this case), so we can't set a conditional style for that case, but I can use a trick to always have the proper rounding. I'll update it.

Another general question - I see that there are no visuals for a few sections on the button and typography pages. Assume the gray placeholder boxes would be replaced with these at a later time? Let me know if you need anything from design for this.

Yeah, I just haven't prioritized filling those in. Unlike most style guides, where those boxes contain images sliced from a high-fidelity mockup, ours are currently live views. We'll probably want to switch to images at some point, but doing that responsively is going to be a challenge. Just trying to move quickly at the moment.

@soumya-ashok
Copy link

I wonder if the button should become more of a square with rounded corners if the label were to wrap? The shape of the fully rounded button seems compromised when this happens.

Oh, I meant to fix that. CSS doesn't tell us when the contents have overflowed (wrapped, in this case), so we can't set a conditional style for that case, but I can use a trick to always have the proper rounding. I'll update it.

Great!

Another general question - I see that there are no visuals for a few sections on the button and typography pages. Assume the gray placeholder boxes would be replaced with these at a later time? Let me know if you need anything from design for this.

Yeah, I just haven't prioritized filling those in. Unlike most style guides, where those boxes contain images sliced from a high-fidelity mockup, ours are currently live views. We'll probably want to switch to images at some point, but doing that responsively is going to be a challenge. Just trying to move quickly at the moment.

Okay, happy to give you images once I have the sticker sheet built out.

@dpatil-magento
Copy link
Contributor

Verification steps pass.

@soumya-ashok
Copy link

The changes are UX approved.

@dpatil-magento dpatil-magento self-requested a review March 19, 2020 19:46
@m2-community-project m2-community-project bot moved this from Ready for Review to Reviewer Approved in Pull Request Progress Mar 19, 2020
@dpatil-magento dpatil-magento merged commit 3685e3e into develop Mar 19, 2020
@dpatil-magento dpatil-magento deleted the SG-PWA-433 branch March 19, 2020 19:47
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-styleguide version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants