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

Show all of the split values in shorthand input for SplitChainedNumberInput #6140

Open
1 task
lankaukk opened this issue Jul 29, 2024 · 4 comments
Open
1 task
Labels
Inspector Affects the inspector, settings, commands Perceived Bug Something that feels broken or like a problem

Comments

@lankaukk
Copy link
Collaborator

lankaukk commented Jul 29, 2024

Note: The first half of the original issue has been moved into #6220 and completed

The border radius and padding controls in the inspector both use the SplitChainedNumberInput so that you can choose edit the shorthand css property, or each value individually.

However, when you set a shorthand property and then you condense it back to one value, it feels buggy because it appears as if no value for that property is set anymore. For example:

Image
Image

Solution:

  • show all of the split values in the shorthand input when applied, separated by commas. You should be able to see this and type into the inputs this way for both padding and border-radius: (like padding in figma)

Image
Image
Image

@lankaukk lankaukk added Perceived Bug Something that feels broken or like a problem Inspector Affects the inspector, settings, commands labels Jul 29, 2024
@liady liady self-assigned this Aug 8, 2024
lankaukk pushed a commit that referenced this issue Aug 12, 2024
**Problem:**
the order of the padding controls is incorrect, it follows the order of
the border radius controls but should be a bit different.
this is the first part of #6140

**Fix:**
allow changing the order of the padding controls without changing the
order of the border controls (passing it from outside)

![Image](https://github.com/user-attachments/assets/1a5b3abe-d710-4ee1-8a3c-615fb6b14133)

**Manual Tests:**
I hereby swear that:

- [X] I opened a hydrogen project and it loaded
- [X] I could navigate to various routes in Preview mode

Fixes #6220 that was extracted from #6140
@liady
Copy link
Contributor

liady commented Aug 25, 2024

@lankaukk regarding the second item - do we still want it this way? I feel like we need to decide first how we handle shorthand/expanded representation of these css props

@lankaukk
Copy link
Collaborator Author

@liady yes we still want it this way in the end. The way we handle the css props warrants a dev discussion 👍

@liady
Copy link
Contributor

liady commented Aug 25, 2024

@lankaukk sounds good. So maybe I'll split this ticket to two different issues, so it will be easy to track (one of them is already in its own ticket)

@Rheeseyb Rheeseyb changed the title Improve the split padding and border radius controls Show all of the split values in shorthand input for SplitChainedNumberInput Sep 3, 2024
@balazsbajorics
Copy link
Contributor

hmm, I don't think I like this, this essentially introduces a second parallel way to edit the 4 properties individually!
We have two more options IMO:

  1. when the user changes the control to show fewer inputs, we could indicate the presence of mixed value (usually a grey -), and let them override with a single number
  2. OR we turn the clicking of the (-) icon into a command which actually strips the values and replaces them with a common one.
    to me 1 feels less agressive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Inspector Affects the inspector, settings, commands Perceived Bug Something that feels broken or like a problem
Projects
None yet
Development

No branches or pull requests

3 participants