-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[RNMobile] Add radial gradient infrastructure #22493
Conversation
Size Change: +18.8 kB (1%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Generally, I was following web behaviour so please let me know if that's correct or let me know what should be changed and what is desired behaviour:
GIF:
GIF:
@iamthomasbishop Please, let me know also if you have any other comments about the current solution. |
@lukewalczak Just tested out the build and I jotted down some notes:
In both of these cases — if we go with my first suggestion above, the selected gradient swatch would change to look like the customized gradient. In other words, the user is just customizing/overriding a default gradient. In the future when we add more custom gradients, we can decide to either keep this "override" behavior or limit it and only let the user create custom ones if they want a customized version of a default one (I think the former makes more sense). Does all of this make sense? If not, happy to answer questions. And if you have alternatives in mind or suggestions, let me know. |
I was thinking about it, good to hear that you're suggesting it!
I see :| Generally, I would like to avoid another nesting level (it's going to deep from general settings to background color to customize background to choose proper type). Maybe we should have both options available on Otherwise, if we are going to nest gradient type options, how that subsheet look like?
Thanks for reporting that, will investigate.
Clear, thanks! |
I was thinking about how we might do this as well. I also thought about maybe using a segmented control on the right side of that row, but idk. Maybe we can try that and see how it looks? Obviously we could run into long label issues and if we add more types we'd want to pivot, but that might work for now. |
@lukewalczak I'm sorry, that was a terrible suggestion on my part, definitely looks really weird 🙈. After thinking about it a little bit, I think we could just stack the gradient types in 2 separate table rows and then put the conditional |
d21aefe
to
cb62eae
Compare
Looking pretty good! The switching between linear/radial feels pretty good. My one concern (which I'm not surprised by but wanted to see if it felt this way on a live build) — splitting into sections with section headings does take up a decent amount of space. Can we try removing the "Gradient Angle" section heading and if necessary add ~8px of spacing between the sections to provide some separation without taking up so much space. That would look something like this:
Ah that's a good question. I would expect my customized gradient to stay in place. But if I reset a gradient to its default (which we can't currently do unless you tap on the already-selected swatch. More on that topic below 👇 ), it would reset to that "new" version of the base gradient. This makes me wonder if we should (as I suggested earlier) add the customized gradients to the end of the list as a "custom" swatch? It would solve this issue and the next point below. Note: We also wouldn't need a "reset" button because you'd simply be able to switch "back" to the base gradient by tapping on it.
I find this odd. I would expect it to stay in its current state, same as happens when you tap on an already-selected solid color or tap twice on a base gradient before customizing. Some other notes:
|
How "custom" swatch should look like? Should we reuse custom swatch from solid palette? I assume that adding custom swatch is equal with removing and pressing the custom swatch will move user to dedicated subsheet? I think that solution is the best and will solve a lot of concerns ✌️
It's related to slider behaviour. There is ongoing work on that in separate PR. |
One more question - should that custom swatch be visible only when gradient color is selected or it should be always visible but disabled? |
Can we use the customized gradient as the swatch background? For example if I customize the angle of a base gradient, I would expect that new “custom” swatch to see that new angle w/ the “selected” icon on top of it. Does that make sense?
Yea, if we go with this custom swatch flow, I think so. What we’re essentially doing is allowing the user to “fork” the base gradient into customized version and making it very obvious visually. Here is how I’m seeing the flow in my head: Just in case the sketch isn’t clear:
If we instead wanted to keep the same swatch (position) selected instead of adding a new one at the end of the list, I have a good idea how we might tackle that — I think it’d mostly rely on a “reset” button to make it clear what’s going on.
I think we might want an additional 8 or 16px spacing — it looks a bit too close there. |
338a302
to
74bbde9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Tested this on both platforms and it's working correctly, nice work Luke!! I just left a couple of comments.
packages/components/src/mobile/bottom-sheet/color-cell.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet/navigation-header.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/segmented-control/index.native.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job Luke! 🎉 I left some small comments but everything works as expected :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested it again after the changes. Nice work Luke!
Description
That PR is adding infrastructure for gradients to switch between both types along with changing a linear gradient angle.
That PR is based on initial radial gradient PR
Ref to gb-mobile: wordpress-mobile/gutenberg-mobile#2277
Testing ios: wordpress-mobile/WordPress-iOS#14154
Testing android: wordpress-mobile/WordPress-Android#11999
How has this been tested?
gradient
segmentExpect:
Customize Gradient
control appearedCustomize Gradient
Angle
Expect:
Radial
typeExpect:
back
buttonExpect:
Custom
text along with selected and customized gradient appearedsolid
segmentgradient
segmentExpect:
Custom
or selected color swatchExpect:
Customize Gradient
subsheet is openedScreenshots
Types of changes
Adding new control in color settings called
Customize Gradient
which moves into subsheet with controls to change gradient type and linear gradient angle.Checklist: