Set custom color when applying initial background image #54054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
draganescu
added
[Type] Enhancement
|
Size Change: +606 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
draganescu
force-pushed
the
try/colorthief
branch
from
63e1426
to
21c0164
Compare
ajlende
requested changes
Aug 31, 2023
|
@ajlende added the dark check after the media sets custom color. |
ajlende
requested changes
Aug 31, 2023
|
@ajlende did some refactoring to use retrieving average color functions once. Got rid of an |
draganescu
force-pushed
the
try/colorthief
branch
from
12132cb
to
3787118
Compare
ajlende
requested changes
Sep 2, 2023
draganescu
force-pushed
the
try/colorthief
branch
from
a822e6b
to
999b49c
Compare
|
I am starting over this one. |
|
There are a few things to try and achieve with this:
(2.1) and (2.2) make refactoring annoying because:
|
draganescu
commented
Sep 14, 2023
ajlende
reviewed
Sep 14, 2023
| } ); | ||
| setIsDark( overlayColor.color, await getAverageBackgroundColor( url ) ); |
There was a problem hiding this comment.
Moving the await below the other setAttributes is going to cause the attributes to be update in two steps with a delay vs one step before. I haven't run the code yet, but I expect this will trigger two renders instead of one.
The effect of everything else being updated and then the isDark setting lagging behind will be more obvious on slower hardware. Moving the await to the top will probably have the same behavior as before, but I'm not 100% certain because setAttributes is weird and it would still get called twice, just closer together.
Same thing for onSetOverlayColor and onUpdateDimRatio.
There was a problem hiding this comment.
also good to think about making atomic updates so that state is never partially updated and thus inconsistent. we definitely only want one undo/redo point for this operation, if any at all (which I guess we might not want an undo step since this flows from the other user-initiated action?)
There was a problem hiding this comment.
I updated some places, but I had to draw the line somewhere with all the refactoring that was required. And the line I drew was refactoring out withColors which is a HOC that adds the setOverlayColor prop which calls setAttributes.
I made sure that everywhere that setOverlayColor was followed by another setAttributes, there was an __unstableMarkNextChangeAsNotPersistent() call to keep the undo levels to what you would expect while editing.
|
Addressed @ajlende latest review and fixed the behavior to respect the manually set color overlay on load. Also tweaked the use featured image behavior. Updated the OP with a video with current working state. Following a conversation with @richtabor there is the question of when should the overlay be set from the average color of the background image? All the time when a background image is applied or only if there is no overlay color? Or only when the overlay color is not manually set? Ideally we would know the difference between an overlay color manually set by a user and one set by the system. And in this PR we do, but only until the page is reloaded. The best simplification would be to set the overlay from average color of background image only if there is no overlay color and no background image. Otherwise we'd have to save a new attribute, so that when the user switches the background image we can tell if the color is the one from the previous image average color, or if it's set manually, and on media change only suggest if the color is set by the system. We need to decide between:
|
I think this is the correct path, though it can be in a follow-up to get this in. I would expect an overlay color suggested as long as I didn’t manually change the color myself. If I set an overlay color, that color should persist on media change. |
I wonder if we could use the synchronous FAC API on save when an |
…scribe what it means in the contexts used
draganescu
force-pushed
the
try/colorthief
branch
from
819e216
to
a3cb7f8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #44167
The original attempt was using
colorThief(#22564) and there is another PR (#46908) which I learned after doing this one doing the same thing.Anyway 😬 this PR aims to land the feature.
cover-block-auto-overlay.mp4