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.

Sign up for GitHub

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

Jump to bottom

Add color section to styleguide #2030

Merged
merged 15 commits into from
Jan 16, 2020
Merged

Add color section to styleguide #2030

merged 15 commits into from
Jan 16, 2020

Conversation

jimbo
Copy link
Contributor

@jimbo jimbo commented Dec 12, 2019

Description

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

Related Issue

PWA-224

Acceptance

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

Verification Stakeholders

Specification

Verification Steps

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

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 version: Minor This changeset includes functionality added in a backwards compatible manner. in progress labels Dec 12, 2019
@jimbo jimbo changed the base branch from develop to jimbo/styleguide-typography December 12, 2019 23:44
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Dec 12, 2019 •
edited
Loading

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

Generated by 🚫 dangerJS against 26ddce2

@jimbo jimbo changed the base branch from jimbo/styleguide-typography to develop January 3, 2020 21:27
@jimbo jimbo removed the in progress label Jan 6, 2020
supernova-at
supernova-at previously approved these changes Jan 8, 2020
View reviewed changes
@dpatil-magento
Copy link
Contributor

Verification steps and regression pass but need @jcalcaben and @soumya-ashok review to get this merged.

jcalcaben
jcalcaben previously approved these changes Jan 9, 2020
View reviewed changes
revanth0212
revanth0212 reviewed Jan 10, 2020
View reviewed changes
packages/venia-styleguide/src/components/Palette/Tone.js Outdated
const getLightness = channels => {
const [r, g, b] = channels.map(getSRGB);

return r * 0.2126 + g * 0.7152 + b * 0.0722;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you drop a comment about these numbers?

revanth0212
revanth0212 reviewed Jan 10, 2020
View reviewed changes
packages/venia-styleguide/src/components/Palette/Tone.js Outdated

return factor <= 0.03928
? factor / 12.92
: Math.pow((factor + 0.055) / 1.055, 2.4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you drop a comment about these numbers?

revanth0212
revanth0212 reviewed Jan 10, 2020
View reviewed changes
Copy link
Contributor

@revanth0212 revanth0212 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Couple questions around numbers used. Dropping some comments will help.

@jimbo jimbo dismissed stale reviews from jcalcaben and supernova-at via ee11af7 January 14, 2020 18:35
@jimbo
Copy link
Contributor Author

jimbo commented Jan 14, 2020

Couple questions around numbers used. Dropping some comments will help.

Thanks, good catch. I've extracted those utils to a new file and put a comment in there citing the original source. The numbers you highlighted are magic numbers from a formula that I obviously didn't create.

@jimbo jimbo mentioned this pull request Jan 14, 2020
Merged
@jimbo
Copy link
Contributor Author

jimbo commented Jan 16, 2020

Addressed feedback from @soumya-ashok by putting the colors into sections that reflect their usage, and ordering those sections.

@jimbo jimbo merged commit d4c13b7 into develop Jan 16, 2020
@jimbo jimbo deleted the jimbo/styleguide-color branch September 14, 2020 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revanth0212 revanth0212 revanth0212 left review comments

@supernova-at supernova-at supernova-at left review comments

@jcalcaben jcalcaben jcalcaben left review comments

Assignees

@jcalcaben jcalcaben

@supernova-at supernova-at

@revanth0212 revanth0212

Labels
pkg:venia-styleguide version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jimbo @PWAStudioBot @dpatil-magento @jcalcaben @supernova-at @revanth0212