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

Cover Image: Add range slider for background dim #2815

Merged
merged 3 commits into from Oct 2, 2017

Conversation

Projects
None yet
3 participants
@mcsf
Contributor

mcsf commented Sep 27, 2017

Implements #2006

gutenberg-cover-dim-slider-2

(previous screencast)

In terms of design:

  • Should the slider only appear when dimming is toggled? (Current)
  • Should the slider's position or other style reflect is subordinate relationship with the toggle?
  • Should the slider altogether replace the toggle?
  • Should I start this over? :)

In technical terms:

  • The toggle-based dimming was implemented with a CSS rule on a :before element. These aren't programmatically manipulable, so fully dynamic dimming (i.e., setting background for any alpha value) isn't possible. The current solution is based on ten hardcoded steps offered to the user.
  • Is it worth moving away from this? (My gut says no.)
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 27, 2017

Codecov Report

Merging #2815 into master will increase coverage by 0.2%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2815     +/-   ##
=========================================
+ Coverage   33.77%   33.97%   +0.2%     
=========================================
  Files         191      191             
  Lines        5691     6199    +508     
  Branches      996     1136    +140     
=========================================
+ Hits         1922     2106    +184     
- Misses       3189     3421    +232     
- Partials      580      672     +92
Impacted Files Coverage Δ
blocks/library/cover-image/index.js 33.33% <50%> (+2.89%) ⬆️
blocks/library/code/index.js 53.84% <0%> (-3.3%) ⬇️
blocks/editable/index.js 8.92% <0%> (-1.59%) ⬇️
blocks/library/separator/index.js 60% <0%> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/library/preformatted/index.js 50% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
...ditor/modes/visual-editor/invalid-block-warning.js 0% <0%> (ø) ⬆️
blocks/library/video/index.js 21.21% <0%> (+0.15%) ⬆️
... and 10 more

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 e7f1223...7280973. Read the comment docs.

codecov bot commented Sep 27, 2017

Codecov Report

Merging #2815 into master will increase coverage by 0.2%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2815     +/-   ##
=========================================
+ Coverage   33.77%   33.97%   +0.2%     
=========================================
  Files         191      191             
  Lines        5691     6199    +508     
  Branches      996     1136    +140     
=========================================
+ Hits         1922     2106    +184     
- Misses       3189     3421    +232     
- Partials      580      672     +92
Impacted Files Coverage Δ
blocks/library/cover-image/index.js 33.33% <50%> (+2.89%) ⬆️
blocks/library/code/index.js 53.84% <0%> (-3.3%) ⬇️
blocks/editable/index.js 8.92% <0%> (-1.59%) ⬇️
blocks/library/separator/index.js 60% <0%> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
blocks/library/preformatted/index.js 50% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
...ditor/modes/visual-editor/invalid-block-warning.js 0% <0%> (ø) ⬆️
blocks/library/video/index.js 21.21% <0%> (+0.15%) ⬆️
... and 10 more

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 e7f1223...7280973. Read the comment docs.

@melchoyce

This comment has been minimized.

Show comment
Hide comment
@melchoyce

melchoyce Sep 27, 2017

In my opinion...

Should the slider altogether replace the toggle?

Yes, which answers your first two questions. I think we should get rid of the toggle and just show the slider, defaulting to 0. Then, I'd update the label to either "Dimness" or "Opacity" and show the value as a percentage of 100%, rather than 0–1.

melchoyce commented Sep 27, 2017

In my opinion...

Should the slider altogether replace the toggle?

Yes, which answers your first two questions. I think we should get rid of the toggle and just show the slider, defaulting to 0. Then, I'd update the label to either "Dimness" or "Opacity" and show the value as a percentage of 100%, rather than 0–1.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Sep 27, 2017

Contributor

All those sound good!

Contributor

mcsf commented Sep 27, 2017

All those sound good!

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Sep 27, 2017

Contributor

Updated:

gutenberg-cover-dim-slider-2

Contributor

mcsf commented Sep 27, 2017

Updated:

gutenberg-cover-dim-slider-2

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 28, 2017

Member

Noting that it's an inevitably as we iterate on blocks, but this will likely cause many existing Cover Image blocks to become flagged as invalid, due to markup / class name changes.

Related: #2541

Member

aduth commented Sep 28, 2017

Noting that it's an inevitably as we iterate on blocks, but this will likely cause many existing Cover Image blocks to become flagged as invalid, due to markup / class name changes.

Related: #2541

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Sep 28, 2017

Contributor

Thanks for the sanity, @aduth! Feedback addressed.

Contributor

mcsf commented Sep 28, 2017

Thanks for the sanity, @aduth! Feedback addressed.

className,
dimRatioToClass( dimRatio ),
{
'has-background-dim': dimRatio !== 0,

This comment has been minimized.

@aduth

aduth Sep 29, 2017

Member

Was this added to try to preserve backwards compatibility with existing content? I'd have hoped it would be the case, but unfortunately existing cover image blocks appear to be flagged as invalid. The issue appears to be that the old content will have the has-background-dim class, but the dimRatio will not be found and therefore default to 0, thus causing a subsequent save to save without has-background-dim and cause content loss (the validator in action!). Maybe we should assign the default for dimRatio to mimic what had previously existed as the default for hasBackgroundDim effect?

(For debugging validation, see also new #2837)

@aduth

aduth Sep 29, 2017

Member

Was this added to try to preserve backwards compatibility with existing content? I'd have hoped it would be the case, but unfortunately existing cover image blocks appear to be flagged as invalid. The issue appears to be that the old content will have the has-background-dim class, but the dimRatio will not be found and therefore default to 0, thus causing a subsequent save to save without has-background-dim and cause content loss (the validator in action!). Maybe we should assign the default for dimRatio to mimic what had previously existed as the default for hasBackgroundDim effect?

(For debugging validation, see also new #2837)

This comment has been minimized.

@mcsf

mcsf Sep 29, 2017

Contributor

Was this added to try to preserve backwards compatibility with existing content?

Not really. For the issue of compatibility, I stopped to consider what we could do at the framework level. Regardless, something as small as this shouldn't be blocked by that, so I pushed 2884db eab53ef. Note, though, the weirdness of

 function dimRatioToClass( ratio ) {
-       return ratio === 0
+       return ( ratio === 0 || ratio === 50 )
                ? null
                : 'has-background-dim-' + Math.round( ratio / 10 );
 }

Since 50% dimming corresponds to the previous default value of hasBackgroundDim, we need to treat that specifically to avoid altering our block's class list.

@mcsf

mcsf Sep 29, 2017

Contributor

Was this added to try to preserve backwards compatibility with existing content?

Not really. For the issue of compatibility, I stopped to consider what we could do at the framework level. Regardless, something as small as this shouldn't be blocked by that, so I pushed 2884db eab53ef. Note, though, the weirdness of

 function dimRatioToClass( ratio ) {
-       return ratio === 0
+       return ( ratio === 0 || ratio === 50 )
                ? null
                : 'has-background-dim-' + Math.round( ratio / 10 );
 }

Since 50% dimming corresponds to the previous default value of hasBackgroundDim, we need to treat that specifically to avoid altering our block's class list.

This comment has been minimized.

@aduth

aduth Sep 29, 2017

Member

Not really.

If not, why do we need has-background-dim at all? (Edit: I say this, though it may allow us to keep compatibility)

@aduth

aduth Sep 29, 2017

Member

Not really.

If not, why do we need has-background-dim at all? (Edit: I say this, though it may allow us to keep compatibility)

This comment has been minimized.

@aduth

aduth Sep 29, 2017

Member

Note, though, the weirdness of

I think this is fine, expected even. Similar to how we omit default values from serialized comments, we can omit the default value (50) from the class set.

@aduth

aduth Sep 29, 2017

Member

Note, though, the weirdness of

I think this is fine, expected even. Similar to how we omit default values from serialized comments, we can omit the default value (50) from the class set.

This comment has been minimized.

@mcsf

mcsf Sep 29, 2017

Contributor

So... my answer was for the past only. :)

I say this, though it may allow us to keep compatibility

Exactly. At first, it lingered while I pondered on the general compat stuff, but with that eab53ef it makes sense to keep it.

@mcsf

mcsf Sep 29, 2017

Contributor

So... my answer was for the past only. :)

I say this, though it may allow us to keep compatibility

Exactly. At first, it lingered while I pondered on the general compat stuff, but with that eab53ef it makes sense to keep it.

@aduth

aduth approved these changes Sep 29, 2017

Confirmed back-compat, looks great 👍

@mcsf mcsf merged commit eb25d78 into master Oct 2, 2017

3 checks passed

codecov/project 33.97% (+0.2%) compared to e7f1223
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mcsf mcsf deleted the add/cover-image-ranged-dimmer branch Oct 2, 2017

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