Implement gradient outlines #1004
Merged
Conversation
This file contains hidden or 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
|
This is awesome! |
adroitwhiz
force-pushed
the
stroke-gradient
branch
2 times, most recently
from
8aaf088 to
45a8b22
Compare
|
These aren't showing up properly on the stage |
This was referenced May 26, 2020
This was referenced Jun 4, 2020
adroitwhiz
changed the title
adroitwhiz
force-pushed
the
stroke-gradient
branch
from
b0f73cf to
f431b2e
Compare
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
src/reducers/fill-color-2.js
Outdated
| return state; | ||
| // Gradient type may be solid when multiple gradient types are selected. | ||
| // In this case, changing the first color should not change the second color. | ||
| if (colors.gradientType !== GradientTypes.SOLID || colors.fillColor2 === MIXED) { |
There was a problem hiding this comment.
I think we figured out that the getColorsFromSelection function returns GradientType = SOLID, fill colors 1 and 2 = MIXED to indicate that the gradient type isn't actually solid, you've selected a bunch of things with different gradient types. (Just clarifying this for myself)
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 19, 2020
fsih
reviewed
Jun 23, 2020
fsih
reviewed
Jun 23, 2020
fsih
reviewed
Jun 23, 2020
fsih
reviewed
Jun 26, 2020
fsih
reviewed
Jun 26, 2020
Now, you can pass null in for a color instead of
{primary: null, secondary: null, gradientType: GradientTypes.SOLID}
and it'll still clear the color. Passing strokeWidth is also
optional now.
This *should* be safe because OvalTool and RectTool's wrapper "mode" components, the only places that call onSelectionChanged, also update the tools' color, and the color state reducers will always set the color every time the selection changes, meaning it'll be updated anyway.
This means that gradient tools will also be enabled for the stroke color indicator even in fill and bitmap modes, but that's okay because the stroke color indicator will be disabled or hidden in those modes anyway.
Hopefully the comments help; the logic is kinda convoluted here
This fixes the bug where percertly horizontal gradients on perfectly vertical lines and vice versa would not be rendered at all
The UX here isn't the best thing in the world but I think having the functionality is important judging from the playtest
Previously, if you selected a shape with a gradient outline that had a width of 0, its outline color wouldn't be null when it should have been.
- Set the stroke color to "null" when the width is set to 0 - Properly set the stroke color state when the width is increased from 0
adroitwhiz
force-pushed
the
stroke-gradient
branch
from
5e5890d to
cca0832
Compare
|
@fsih The latest 4 commits are new, and should fix issues relating to changing the outline width to 0. Did you run into "something strange" in a local build or my build? It looks like you ran into the bug that scratchfoundation/paper.js#40 fixes, so you may need to |
|
@adroitwhiz Yup, it was a local build. You're right, that's probably what it was! |
adroitwhiz
force-pushed
the
stroke-gradient
branch
from
ede67d9 to
272a10c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Depends on scratchfoundation/paper.js#39, scratchfoundation/paper.js#40, #1166
Resolves
Resolves #540
Diff: https://github.com/adroitwhiz/scratch-paint/compare/stroke-gradient-part-1...adroitwhiz:stroke-gradient?expand=1
Proposed Changes
This PR adds support for (and contains a bunch of supporting changes for) setting outlines' colors to gradients in select mode, fill mode, and shape tools:
The supporting changes include:
style-path.jsto work on both fill and stroke.FillColorPickerComponentandStrokeColorPickerComponentinto aColorPickerComponent.FillColorPickerandStrokeColorPickercontainers into aColorPickercontainer component generator.It deliberately does not yet add support for gradient strokes in fill mode-- there's some behavior there that would best be ironed out at some point in the future.Per design meeting we agreed to include gradient outlines in select mode, gradients in shape tools, and gradient outlines in the fill tool in one release
Reason for Changes
This has been a commonly-requested feature ever since 3.0 was first released to the public.
The supporting changes should also make it easier to implement things like #1003.
Test Coverage
Existing fill/outline color reducer tests have been merged together into one big color reducer test