-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
feat: add pseudo style-selectors #466
Conversation
textProps | ||
| fontProps | ||
| coreStyleProps | ||
| pseudoProps(list(textStyleProps)) |
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.
should this be all props i.e. on hover can we only change the styles that relate to text elements?
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.
My thought was to only accept whatever style-props a Text-element (in this case) can take. But it's possible I'm overlooking some use-cases! 🙂
(I've only begun with the Text-primitive for now, but once there's a solid approach I'll add them to the others as well)
@lessp, I wonder if since the |
Thanks @Akin909, I'll have another look! The main-issue I have with how it works now is the |
e261586
to
c4e0dfb
Compare
@lessp re. having to add |
Cool! I'll have another look-over at this and then implement it to the others as well! 🙂 |
@Akin909, I made some minor adjustments and added hover-styles to most primitives, I did notice that there's a conflict with the If someone has any pointers to what direction we could go here, that would be great! |
I could use this feature in a project that I'm working on right now. Is there anything I can help with to move this along? |
I've paused this a bit @rudolfs, since I'm not sure how this will align itself with the new API #489 When working on the Hackernews-example I've resorted to doing something like this: module InteractableText = {
type state =
| Idle
| Hover
| Active;
type action =
| Idle
| Hover
| Active;
let component = React.component("InteractableText");
let make =
(
~children: list(React.syntheticElement),
~hoverStyles=[],
~activeStyles=[],
~style=[],
~text,
(),
) =>
component(hooks => {
let (state, dispatch, hooks) =
Hooks.reducer(
~initialState=Idle,
(action, _state) =>
switch (action) {
| Idle => Idle
| Hover => Hover
| Active => Active
},
hooks,
);
let currentStyle =
switch (state) {
| Idle => style
| Hover => Style.merge(~source=style, ~target=hoverStyles)
| Active => Style.merge(~source=style, ~target=activeStyles)
};
(
hooks,
<Text
onMouseOut={_ => dispatch(Idle)}
onMouseOver={_ => dispatch(Hover)}
onMouseDown={_ => dispatch(Active)}
style=currentStyle
text
/>,
);
});
let createElement =
(~children, ~hoverStyles=[], ~activeStyles=[], ~style=[], ~text, ()) =>
make(~children, ~hoverStyles, ~activeStyles, ~style, ~text, ());
};
<InteractableText
style=Style.[color(Colors.blue)]
hoverStyles=Style.[color(Colors.red)]
text="Hello!"
/> |
Gotcha. Thanks for the snippet @lessp! |
Closing in favour of a hook-based approach e.g.: #868 I think this is my first PR in Revery. They lure you in... |
EDIT:
So I went on and implemented
active([...])
as well, using the same approach with addingactive
to the Style-type. Which perhaps is still not ideal, but may be good enough. So in my mind, theextractPseudoStyles
-function is the mainly what needs improvements.Draft for brainstorming
Ok, so this is still a work in progress! I've stumbled upon a few things which I'm still trying to get my head around.
Did you have any specific ideas on how to hold state in thePrimitives.re
@Akin909? I just added something to get it to render hover-styles on screen for now (any text-element that is being hovered will apply the styles)Currently to make theStyle.create
-function accept thelist(textStyleProps)
we're forced(?) to addhover
toStyle.t
. This also leads me to add it toapplyStyle
which I've currently made recursive to loop over all the properties of the hover-styles. Right now we could just return style since it's not used at all.Eventually closes #352