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

Block Switcher: Add 'Transform into:' to top of the list #2846

Merged
merged 3 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@christophherr
Contributor

christophherr commented Oct 2, 2017

Description

Adds the text "Transform into" wrapped in span tags to the top of the list in the Block Switcher.
Related issue: #493

How Has This Been Tested?

Visually on my local machine.

Screenshots (jpeg or gifs if applicable):

Before: https://cloudup.com/cPWwpVWU7b5
After: https://cloudup.com/cOKNmDJeu_8

Types of changes

It's labelled as an enhancement.

Checklist:

  • [ x] My code is tested.
  • [ x] My code follows the WordPress code style.
  • My code follows has proper inline documentation.
Polish the transform message
- Add padding
- Add color
- Rephrase to remove the s
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2017

Contributor

Nice! Thank you for working on this.

I took the liberty of pushing a little fix to the visual styles:

screen shot 2017-10-02 at 11 09 29

What do you think? If you're cool with the changes then 👍 👍 from me.

Contributor

jasmussen commented Oct 2, 2017

Nice! Thank you for working on this.

I took the liberty of pushing a little fix to the visual styles:

screen shot 2017-10-02 at 11 09 29

What do you think? If you're cool with the changes then 👍 👍 from me.

@christophherr

This comment has been minimized.

Show comment
Hide comment
@christophherr

christophherr Oct 2, 2017

Contributor

Thanks for reviewing and improving.
The color contrast with $light-gray-900 seems pretty low.
What is the minimum contrast ratio we need?

Contributor

christophherr commented Oct 2, 2017

Thanks for reviewing and improving.
The color contrast with $light-gray-900 seems pretty low.
What is the minimum contrast ratio we need?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 3, 2017

Contributor

The color contrast with $light-gray-900 seems pretty low.

I personally feel like for this particular help text it's okay that it's lower than usual.

However I also would never object to increasing the contrast.

You can see the color variables here: https://github.com/WordPress/gutenberg/blob/master/editor/assets/stylesheets/_variables.scss and use this contrast checker: https://www.joedolson.com/tools/color-contrast.php

In my test it seems like $dark-gray-300 is the lightest one that passes.

If you address Riads feedback, I think this PR is good to merge 👍 👍

Thanks again!

Contributor

jasmussen commented Oct 3, 2017

The color contrast with $light-gray-900 seems pretty low.

I personally feel like for this particular help text it's okay that it's lower than usual.

However I also would never object to increasing the contrast.

You can see the color variables here: https://github.com/WordPress/gutenberg/blob/master/editor/assets/stylesheets/_variables.scss and use this contrast checker: https://www.joedolson.com/tools/color-contrast.php

In my test it seems like $dark-gray-300 is the lightest one that passes.

If you address Riads feedback, I think this PR is good to merge 👍 👍

Thanks again!

@christophherr

This comment has been minimized.

Show comment
Hide comment
@christophherr

christophherr Oct 3, 2017

Contributor

Thanks for your help!
I addressed the feedback from both of you.

Contributor

christophherr commented Oct 3, 2017

Thanks for your help!
I addressed the feedback from both of you.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 3, 2017

Codecov Report

Merging #2846 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2846      +/-   ##
=========================================
+ Coverage   33.74%   33.9%   +0.15%     
=========================================
  Files         191     191              
  Lines        5695    5699       +4     
  Branches      997     999       +2     
=========================================
+ Hits         1922    1932      +10     
+ Misses       3192    3187       -5     
+ Partials      581     580       -1
Impacted Files Coverage Δ
editor/block-switcher/index.js 0% <ø> (ø) ⬆️
blocks/library/more/index.js 22.22% <0%> (-7.78%) ⬇️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/library/paragraph/index.js 33.33% <0%> (ø) ⬆️
...ditor/sidebar/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98% <0%> (+0.08%) ⬆️
editor/reducer.js 88.96% <0%> (+0.39%) ⬆️
blocks/library/cover-image/index.js 33.33% <0%> (+2.89%) ⬆️
blocks/api/paste/index.js 86.66% <0%> (+7.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a596ceb...0e78f8f. Read the comment docs.

codecov bot commented Oct 3, 2017

Codecov Report

Merging #2846 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2846      +/-   ##
=========================================
+ Coverage   33.74%   33.9%   +0.15%     
=========================================
  Files         191     191              
  Lines        5695    5699       +4     
  Branches      997     999       +2     
=========================================
+ Hits         1922    1932      +10     
+ Misses       3192    3187       -5     
+ Partials      581     580       -1
Impacted Files Coverage Δ
editor/block-switcher/index.js 0% <ø> (ø) ⬆️
blocks/library/more/index.js 22.22% <0%> (-7.78%) ⬇️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/library/paragraph/index.js 33.33% <0%> (ø) ⬆️
...ditor/sidebar/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98% <0%> (+0.08%) ⬆️
editor/reducer.js 88.96% <0%> (+0.39%) ⬆️
blocks/library/cover-image/index.js 33.33% <0%> (+2.89%) ⬆️
blocks/api/paste/index.js 86.66% <0%> (+7.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a596ceb...0e78f8f. Read the comment docs.

@youknowriad

Nice! thank you

@youknowriad youknowriad merged commit 6dc6c78 into WordPress:master Oct 3, 2017

2 checks passed

codecov/project 33.9% (+0.15%) compared to a596ceb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@christophherr christophherr deleted the christophherr:fix/493 branch Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment