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

Updated color picker clear and created a color indication in title. #3054

Merged
merged 4 commits into from Nov 15, 2017

Conversation

@jorgefilipecosta
Member

jorgefilipecosta commented Oct 18, 2017

Description

This PR tries to close issue #2770. By updating color picker to have a clear button in link style. And have a color indication in the panel titles.

Testing

  1. Add a paragraph block, choose a color for text and background, verify the color appears at the side of the title.
  2. Repeat for the button block.
  3. Collapse the color panels and verify the color indication persists.
  4. Use the clear button and verify it clears the color changes.

Screenshots

screen shot 2017-10-18 at 16 19 34

screen shot 2017-10-18 at 16 20 07

Show outdated Hide outdated components/panel/body.js
Show outdated Hide outdated components/panel/style.scss
@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Oct 24, 2017

Member

Visually 👍 for the design. I think you are waiting for a code review but great work getting it like the mockups.

Member

karmatosed commented Oct 24, 2017

Visually 👍 for the design. I think you are waiting for a code review but great work getting it like the mockups.

Show outdated Hide outdated components/panel/style.scss
Show outdated Hide outdated components/panel/body.js
Show outdated Hide outdated components/panel/body.js

@mtias mtias added the Accessibility label Oct 25, 2017

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Oct 25, 2017

Member

Hi @aduth, I followed your sugestions and simplified the css, thank you :)

Member

jorgefilipecosta commented Oct 25, 2017

Hi @aduth, I followed your sugestions and simplified the css, thank you :)

@aduth

Small issue: When a color value is assigned, the height of the toggle is adjusted slightly. We should try to avoid this:

resize

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Oct 27, 2017

Contributor

It might be nice to default to closed panels for text / background color now.

Contributor

mtias commented Oct 27, 2017

It might be nice to default to closed panels for text / background color now.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Oct 27, 2017

Member

I updated the PR solving the small move identified by @aduth and I made color panels hidden by default as @mtias suggested.

Member

jorgefilipecosta commented Oct 27, 2017

I updated the PR solving the small move identified by @aduth and I made color panels hidden by default as @mtias suggested.

Show outdated Hide outdated components/panel/color.js
Show outdated Hide outdated components/panel/color.js

@mtias mtias added this to the Beta 1.6 milestone Oct 31, 2017

@youknowriad

Nice 👍

@@ -3,6 +3,11 @@ $color-palette-circle-spacing: 14px;
.blocks-color-palette {
margin-right: -14px;
.blocks-color-palette__clear {

This comment has been minimized.

@youknowriad

youknowriad Oct 31, 2017

Contributor

Should we avoid nesting here? We tend to favor top-level rules when possible.

@youknowriad

youknowriad Oct 31, 2017

Contributor

Should we avoid nesting here? We tend to favor top-level rules when possible.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Oct 31, 2017

Member

Hi, the reason why this one is nested is for the CSS rule to have higher specificity. If the rule is not nested the margin is overwritten.

@jorgefilipecosta

jorgefilipecosta Oct 31, 2017

Member

Hi, the reason why this one is nested is for the CSS rule to have higher specificity. If the rule is not nested the margin is overwritten.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Oct 31, 2017

Member

Hi @youknowriad thank you for the review!

Member

jorgefilipecosta commented Oct 31, 2017

Hi @youknowriad thank you for the review!

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Oct 31, 2017

Member

Hi, @aduth, @mtias, @youknowriad, I added the changes to make colors collapsed in the paragraph and expanded in button. In order to make things work nicely, I added a simple change in panel body that resets the state when initialOpen prop changes. The changes are in a new commit, I tested and things are looking ok.

Member

jorgefilipecosta commented Oct 31, 2017

Hi, @aduth, @mtias, @youknowriad, I added the changes to make colors collapsed in the paragraph and expanded in button. In order to make things work nicely, I added a simple change in panel body that resets the state when initialOpen prop changes. The changes are in a new commit, I tested and things are looking ok.

@mtias mtias removed this from the Beta 1.6 milestone Oct 31, 2017

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 4, 2017

Member

Hi @aduth, @youknowriad, I researched more but the keys problem that was affecting this PR. It looks like every component we render in fill is "copied" to the slot by the slot-fill library, during this process the keys that the elements have are ignored and new sequential ones are assigned. That's is the reason why even setting keys on ColorPanels did not manage to make the components remount.
The children of the components passed to fill keep their keys in slot, so In the InspectorControls component, I added a div around the children. This div will have an assigned key by slot-fill but its children (the components we pass to InspectorControls) keep their id's. I assigned unique id's to ColorPanels in paragraph and button and now things are working as they should :)

One possible general solution to always force remount between blocks is making the elements pass and identifier prop to InspectorControls, then InspectorControls renders two wrappers one without the key (will have the key assigned by slot-fill) and one inside with key equal to unique identifier passe. And then inside the second wrapper, we render the children. It's not a very clean solution but it's simple and should work.

Member

jorgefilipecosta commented Nov 4, 2017

Hi @aduth, @youknowriad, I researched more but the keys problem that was affecting this PR. It looks like every component we render in fill is "copied" to the slot by the slot-fill library, during this process the keys that the elements have are ignored and new sequential ones are assigned. That's is the reason why even setting keys on ColorPanels did not manage to make the components remount.
The children of the components passed to fill keep their keys in slot, so In the InspectorControls component, I added a div around the children. This div will have an assigned key by slot-fill but its children (the components we pass to InspectorControls) keep their id's. I assigned unique id's to ColorPanels in paragraph and button and now things are working as they should :)

One possible general solution to always force remount between blocks is making the elements pass and identifier prop to InspectorControls, then InspectorControls renders two wrappers one without the key (will have the key assigned by slot-fill) and one inside with key equal to unique identifier passe. And then inside the second wrapper, we render the children. It's not a very clean solution but it's simple and should work.

@ahmadawais

This comment has been minimized.

Show comment
Hide comment
@ahmadawais

ahmadawais Nov 5, 2017

Contributor

Folks, I think the white color that appears from the center on focus is actually quite confusing and we should do away with it.

Contributor

ahmadawais commented Nov 5, 2017

Folks, I think the white color that appears from the center on focus is actually quite confusing and we should do away with it.

title={ [
<span className="components-panel__color-title" key="title">{ title }</span>,
colorValue && <span className="components-panel__color-area" key="color" style={ { background: colorValue } } />,
] }

This comment has been minimized.

@afercia

afercia Nov 5, 2017

Contributor

I like the overall idea to show the selected color. However, this is just a visual indication. There's no text or label that can be read by software. It would be nice to find a way to add some meaningful ext, whether it's visually hidden text or an aria-label attribute to override the button text.

@afercia

afercia Nov 5, 2017

Contributor

I like the overall idea to show the selected color. However, this is just a visual indication. There's no text or label that can be read by software. It would be nice to find a way to add some meaningful ext, whether it's visually hidden text or an aria-label attribute to override the button text.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 6, 2017

Member

Hi @ahmadawais, thank you for your suggestion. This PR adds a color indication in the title making it easier for the user to know which color is selected, so it mitigates the problem you are referring. Although changing the way we show selected color on the pallet is something we can discuss and try to improve in the future.

Hi @afercia thank you a lot for your inputs. I addressed most of them including making color labels be "Background Color"/"Text Color" for both paragraph and button. These ones I'm not certain if for now, we should just use this labels for aria and not display color in title label because we are already showing a color cc: @mtias. Changes are in separate commits so we can easily change them or revert parts of them if needed.
Regarding your suggestions to improve aria labels for clear buttons and provide a color indication as an aria-label, I would like to address them in a separate PR because adding them here would make this PR big and harder to review.

Member

jorgefilipecosta commented Nov 6, 2017

Hi @ahmadawais, thank you for your suggestion. This PR adds a color indication in the title making it easier for the user to know which color is selected, so it mitigates the problem you are referring. Although changing the way we show selected color on the pallet is something we can discuss and try to improve in the future.

Hi @afercia thank you a lot for your inputs. I addressed most of them including making color labels be "Background Color"/"Text Color" for both paragraph and button. These ones I'm not certain if for now, we should just use this labels for aria and not display color in title label because we are already showing a color cc: @mtias. Changes are in separate commits so we can easily change them or revert parts of them if needed.
Regarding your suggestions to improve aria labels for clear buttons and provide a color indication as an aria-label, I would like to address them in a separate PR because adding them here would make this PR big and harder to review.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Nov 6, 2017

Contributor

@jorgefilipecosta thanks!

we should just use this labels for aria and not display color in title label because we are already showing a color

Visible labels help everyone, not just assistive technologies users. I'd recommend to keep the word "Color". What was the original reasoning behind removing "Color"? Make space for the color rectangle? :) Then I'd say we're removing relevant information just for visual purposes.

Contributor

afercia commented Nov 6, 2017

@jorgefilipecosta thanks!

we should just use this labels for aria and not display color in title label because we are already showing a color

Visible labels help everyone, not just assistive technologies users. I'd recommend to keep the word "Color". What was the original reasoning behind removing "Color"? Make space for the color rectangle? :) Then I'd say we're removing relevant information just for visual purposes.

@ahmadawais

This comment has been minimized.

Show comment
Hide comment
@ahmadawais

ahmadawais Nov 6, 2017

Contributor

@jorgefilipecosta

Although changing the way we show selected color on the pallet is something we can discuss and try to improve in the future.

That's what I want. And I think it would be the right decision. 💯

Contributor

ahmadawais commented Nov 6, 2017

@jorgefilipecosta

Although changing the way we show selected color on the pallet is something we can discuss and try to improve in the future.

That's what I want. And I think it would be the right decision. 💯

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

Hi @aduth, @youknowriad given the merge of #3404. I rebased and simplified this by removing all slot-fill related hacks. If you could have a last look at merging it would be perfect :) Then I will continue to iterate and address some more other a11y issues.

Member

jorgefilipecosta commented Nov 9, 2017

Hi @aduth, @youknowriad given the merge of #3404. I rebased and simplified this by removing all slot-fill related hacks. If you could have a last look at merging it would be perfect :) Then I will continue to iterate and address some more other a11y issues.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 10, 2017

Codecov Report

Merging #3054 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3054      +/-   ##
==========================================
- Coverage   34.61%   34.61%   -0.01%     
==========================================
  Files         260      261       +1     
  Lines        6757     6758       +1     
  Branches     1226     1226              
==========================================
  Hits         2339     2339              
- Misses       3728     3729       +1     
  Partials      690      690
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 32% <ø> (ø) ⬆️
blocks/library/button/index.js 11.11% <ø> (ø) ⬆️
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
components/panel/color.js 0% <0%> (ø)

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 5255c93...da8f0a2. Read the comment docs.

codecov bot commented Nov 10, 2017

Codecov Report

Merging #3054 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3054      +/-   ##
==========================================
- Coverage   34.61%   34.61%   -0.01%     
==========================================
  Files         260      261       +1     
  Lines        6757     6758       +1     
  Branches     1226     1226              
==========================================
  Hits         2339     2339              
- Misses       3728     3729       +1     
  Partials      690      690
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 32% <ø> (ø) ⬆️
blocks/library/button/index.js 11.11% <ø> (ø) ⬆️
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
components/panel/color.js 0% <0%> (ø)

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 5255c93...da8f0a2. Read the comment docs.

@aduth

aduth approved these changes Nov 14, 2017

Merge conflicts, but otherwise 👍

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 14, 2017

Member

Thank you for your review @aduth, merge conflicts were solved :)

Member

jorgefilipecosta commented Nov 14, 2017

Thank you for your review @aduth, merge conflicts were solved :)

@jorgefilipecosta jorgefilipecosta merged commit cf8fec9 into master Nov 15, 2017

3 checks passed

codecov/project 34.61% (-0.01%) compared to 5255c93
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/updated-color-picker-clear-link-title branch Nov 15, 2017

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