Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add typography section to styleguide #2013

Merged
merged 10 commits into from
Dec 20, 2019

Conversation

jimbo
Copy link
Contributor

@jimbo jimbo commented Dec 4, 2019

Description

Add a typography section to the styleguide. The prose content may be lorem ipsum, but the values should reflect the the official Venia styles.

Related Issue

PWA-153

Acceptance

  • @jcalcaben should affirm document structure
  • @soumya-ashok should affirm Venia typographic styles
  • Others may review the code

Verification Stakeholders

@jcalcaben @soumya-ashok

Specification

Verification Steps

  1. yarn workspace @magento/venia-styleguide run start
  2. Navigate to /page/typography

Screenshots / Screen Captures (if appropriate)

Checklist

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

@jimbo jimbo added in progress version: Minor This changeset includes functionality added in a backwards compatible manner. labels Dec 4, 2019
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Dec 4, 2019

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-153.

Generated by 🚫 dangerJS against 886abe7

@jimbo jimbo marked this pull request as ready for review December 11, 2019 19:08
@jimbo jimbo removed the in progress label Dec 11, 2019
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

So in general this looks great -- are you going to eventually add examples to the left/right as done on the spectrum typography page? Is this a follow-up ticket or should we hold up this PR on that?

https://spectrum.adobe.com/page/typography

</Columns>
<Columns reverse={true}>
<Emphasis />
</Columns>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird -- should the text be contained within the grey-box background?

Image from Gyazo

<Section title="Formatting">
<Formatting />
</Section>
<Section title="Usage guidelines" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is empty. Drop some lorem text placeholder?

***

<Columns>
<FontSize />
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's the columns component but it looks like it makes two columns differing by background.

@@ -0,0 +1,4 @@
.title {
font-size: var(--venia-global-text-fontSize-1100);
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it should use a typography token for heading but I didn't find one that used 1100.

export const H3 = props => <Heading {...props} level={3} />;
export const H4 = props => <Heading {...props} level={4} />;
export const H5 = props => <Heading {...props} level={5} />;
export const H6 = props => <Heading {...props} level={6} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't seem to be used anywhere. Are these supposed to be components for the styleguide UI or for use in PWA Studio (why not both?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nvm I see the MDXAdapter :)

@dpatil-magento
Copy link
Contributor

Verification steps looks good. Need @jcalcaben @soumya-ashok approval to merge this one.

@jimbo jimbo deleted the jimbo/styleguide-typography branch September 14, 2020 21:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:venia-styleguide version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants