Add link element :hover interactivity control to global styles UI
#41976
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
getdave
added
the
Global Styles
getdave
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
|
Size Change: +225 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
getdave
changed the title
getdave
requested review from
scruffian,
MaggieCabrera,
mikachan,
adamziel and
draganescu
This comment was marked as outdated.
This comment was marked as outdated.
|
This is amazingly simple, great job. |
Yes! But due to the way the overlapping is handled the text is offset too far. @scruffian Can you think of a way to avoid this without setting a matching negative margin on the text?
|
On my proposal I suggested adding arrows in case there were more than 3 options:
Something that occurred to me while testing your PR is whether it would make sense to indicate the selectors that have values set. Maybe we could increase the weight of the labels so users don't need to click on every tab to see if there's a color selected. |
javierarce
added
the
Needs Figma Update
getdave
force-pushed
the
add/link-interactivity-controls-global-styles
branch
from
1fc294c to
20d7d0c
Compare
|
I love where this is going, but at the same time I feel like it's over-complicating some things. Another thing that troubles me - but is not related directly to this PR - is that color is not - and should never be - the only differentiator for link statuses. A link on hover can have a dashed outline for example, when it's active it can have a solid outline, when it's visited it's usually (but not always) the same as the default state and so on. Of course these things should be defined in the theme styles, however if we start adding colorpickers for all link statuses, then it's reasonable to assume that at some point in the (very) near future, we'll need to add more controls in link elements like selecting underline/outline/border/shadow/whatever for each link-status 🤔 |
|
Thanks for the thought provoking feedback @aristath 🙇♂️
I'm concerned about exposing I'm more than happy to reduce the prominence of other less utilised pseudo classes. The
Agreed. In Theme JSON you can add
For the time being we're looking to keep the scope of this PR pretty small. Once we're happy with with UI and UX we can then look to expand this to cover more properties and elements. |
getdave
force-pushed
the
add/link-interactivity-controls-global-styles
branch
from
20d7d0c to
02c39d9
Compare
I think that with the arrow solution we could achieve something similar to what @aristath suggested without the need to introduce more elements to the UI. In my original proposal, I also suggested an alternative way of displaying the controls that is probably better than my original one. If we applied it here, we could also use the description to explain each property: Screen.Recording.2022-06-29.at.16.44.57.mov |
|
It looks like the following changes will cause this PR to increase in complexity:
Therefore we should consider going back to just supporting Any objections? |
|
The ZStack should be showing just default and hover colors as well. This is a bit old but shows that even if more states can be set the highlight is just for the most prominent colors:
|
|
@javierarce I really like the mockup and the way it hides the complexity until you actually need it. I'll look to ship this PR in a super simple form and circle back to implement the version you have envisaged. |
getdave
changed the title
:hover interactivity control to global styles UI
getdave
changed the title
:hover interactivity control to global styles UI:hover interactivity control to global styles UI
getdave
commented
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
getdave
commented
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
getdave
commented
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
6 tasks
draganescu
reviewed
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Jun 30, 2022
packages/edit-site/src/components/global-styles/screen-link-color.js
Outdated
Show resolved
Hide resolved
scruffian
reviewed
Jul 1, 2022
Co-authored-by: Andrei Draganescu <[email protected]>
getdave
force-pushed
the
add/link-interactivity-controls-global-styles
branch
from
89ab25e to
395f632
Compare
|
Let's get the tests green! |
57 tasks
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.
What?
Building on foundation put in place in #41786, this PR adds UI to allow users to set interactivity states for the top level link element for:
focuswe decided to simply and handle:hoveronly for now.activewe decided to simply and handle:hoveronly for now.As
these arethis is currently the only value supported in the Theme JSON handler this is all we expose for now.Note: we deliberately simplified this initial implementation. See #41976 (comment) for context.
Why?
We should allow users to set global values for link interactivity.
How?
Following designs in #38277 (comment) this PR adds a tab panel and duplicates the existing color components inside of each tab but this time dynamically generating the values and handlers.
Note: this is currently all very hardcoded and not particularly DRY. However I think that is acceptable and we can refactor as and when we add more elements/states.
Testing Instructions
aelements or their respective pseudo selectors.Screenshots or screencast
Screen.Capture.on.2022-06-29.at.16-41-33.mp4