Skip to content
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

experimental: add gradient controller to update color stops using ui #4159

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

JayaKrishnaNamburu
Copy link
Contributor

@JayaKrishnaNamburu JayaKrishnaNamburu commented Sep 25, 2024

Description

The GradientControl helps in displaying the colour stops that are available in the linear-gradient that users added.

There are two knows issues at the moment.

  • The only limitation is. We can't swap the colours when a colour stop crosses an existing colour stop. In some
    tools, the colours are swapped resulting in the swiping of the gradient too.
    This is because of a limitation/bug on the radix side, they pre-sort the slider stops. So, we can never know which crosses an existing stop and changed it's value. Slider ability to disable swap of multiple thumbs radix-ui/primitives#2247

Steps for reproduction

  • Passing a linear-gradient should show all the color-stops in the gradient.
  • The color-hints are displayed at the bottom of the gradient bar. These can't be moved along, more info about it is in the code component. Because, this basically changes the whole gradient just by moving.
  • The color-hints are showed at the bottom of the gradient. Just to notify users about the existence of the hints.
  • Users should be able to move the color-stops along the slider track.
  • The color-stops can be moved using the arrow keys from the keyboard.
Screenshot 2024-09-25 at 10 32 47 AM Screenshot 2024-09-25 at 10 32 43 AM Screenshot 2024-09-25 at 10 32 38 AM

color-change-gradients

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 5de6)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@JayaKrishnaNamburu JayaKrishnaNamburu marked this pull request as ready for review September 27, 2024 08:53
@JayaKrishnaNamburu JayaKrishnaNamburu marked this pull request as draft September 27, 2024 09:16
@JayaKrishnaNamburu JayaKrishnaNamburu self-assigned this Sep 27, 2024
@JayaKrishnaNamburu JayaKrishnaNamburu marked this pull request as ready for review September 28, 2024 10:25
@TrySound
Copy link
Member

TrySound commented Oct 2, 2024

I like how gradient style tries to balance new dots instead of manually centering, maybe not needed for our case
https://github.com/user-attachments/assets/a339afc6-6d18-4597-9eb0-518dcc03b52e

@TrySound
Copy link
Member

TrySound commented Oct 2, 2024

I would add outline for focus-visible only
image

@TrySound
Copy link
Member

TrySound commented Oct 2, 2024

I instinctively do alt+click to delete thumbs

@TrySound
Copy link
Member

TrySound commented Oct 2, 2024

I would do color picker as popover, tooltip feels super unpredictable

@kof
Copy link
Member

kof commented Oct 3, 2024

I instinctively do alt+click to delete thumbs

I think its not a bad idea for us to support this, since our users know this combo

@JayaKrishnaNamburu
Copy link
Contributor Author

I like how gradient style tries to balance new dots instead of manually centering, maybe not needed for our case https://github.com/user-attachments/assets/a339afc6-6d18-4597-9eb0-518dcc03b52e

It is indeed looking good. But the catch is, if a user sets the slider to a custom location. It changes it at the time of rebalancing when a new stop is added. Or is the behaviour is different there ?

@JayaKrishnaNamburu
Copy link
Contributor Author

I would do color picker as popover, tooltip feels super unpredictable

I am using it as PropOver itself.
https://github.com/webstudio-is/webstudio/blob/add-gradient-control/apps/builder/app/builder/features/style-panel/sections/backgrounds/gradient-control.tsx#L320

Seems the events from Slider are little bit unpredictable.

@JayaKrishnaNamburu
Copy link
Contributor Author

The ChevronBigIconUp is removed now in the new icons. Should i use Play icon and try to fill it. Or should we add it back to the list of icons ?

cc @kof

@kof
Copy link
Member

kof commented Jan 3, 2025

Total list of issues:

  • click on a stop handle shouldn't move it
  • click on track in any place should be adding a stop, not move the selected stop, not use an artificial minimal distance to the previous stop
  • simple linear-gradient(red, red) should work
  • tooltip "Add color stop" missing
  • neither figma nor webflow has hints, why do we need them? I don't know.

@kof kof marked this pull request as draft January 11, 2025 10:27
@JayaKrishnaNamburu
Copy link
Contributor Author

click on track in any place should be adding a stop, not move the selected stop, not use an artificial minimal distance to the previous stop

Looks like this is how all the sliders behave too. Its add for me too while working with gradient selector. But, let's see react-aria. Even it moves the closest thumb, when we click somewhere on the range
https://react-spectrum.adobe.com/react-aria/Slider.html#reusable-wrappers

Do you think, it's the actual standard spec and trying to prevent the movement is anti pattern ?

@kof
Copy link
Member

kof commented Jan 11, 2025

@JayaKrishnaNamburu idk if preventing the movement is supported from the spec, like using some preventDefault, but it doesn't really matter, because in our case its not really a slider, we are just trying to see if slider gets us the UX we need to avoid reimplementing accessibility ourselves. So the question is really - can you hide the track or not let it listen to clicks in some other way or not

@JayaKrishnaNamburu
Copy link
Contributor Author

So the question is really - can you hide the track or not let it listen to clicks in some other way or not
Using radix, if we are doing preventDefault for clicks. It's avoiding the thumb movement altogether. But i see the browser based input[range] doesn't have these type of weird behaviours. So, i will give a try with that this weekend. Ranger slider might not be the best choice, as all of them are moving on clicks. Or either stops to move if prevented for clicks.

@kof
Copy link
Member

kof commented Jan 13, 2025

yeah, but you already have your own click position logic, so why not hide the range, avoid radix for these, and based on where the click happened just add the new stop, click - your own logic, drag - slider logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants