-
Notifications
You must be signed in to change notification settings - Fork 81
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
[Web] Add hover support #69
Conversation
I prefer the first approach because it gives users the ability to do more from the theme file itself otherwise the styles for hover and the logic related to the current variant have to be applied in the component code. The biggest downside for the second one to me is handling the hover effects for a variety of variants (mostly buttons/links for me). I've noticed that when trying to apply pseudo selector like styles to components it often leads to some boilerplate/messier code. |
I added support for both approaches, including variant support (single and multiple.) Feel free to try out the normal example app. |
Version
|
I'll test this in my app more tomorrow and merge to master. Based on my usage in the example app, it all works smoothly, so it should be good to go. I'll probably employ the same strategy in my animation library to add hover support. |
This is 🔥 . I'll use it as soon as it gets merged in! |
Sounds good. Mind trying it out with |
I think there might be a bug where it's re-rendering on hovers, even when there are no hover styles specified. Once I fix that, it'll be good to go. |
Yeah, will do. |
Just fixed that, silly bug, forgot the word |
|
Looks like this is causing a few style issues for me in my real app. I'll have to debug further. At first glance, it seems like padding and align items/self are experiencing weird behavior. I wonder if this is due to a change in the order in |
Does this need to be ported into #74? I can help debug this as well. |
Yeah probably, the one issue is that it has a bug I haven't been able to identify yet. Ideally it would point at that PR, but still be a separate branch until we figure out that bug. |
@cmaycumber Now that #74 is merged, I might take another stab at getting this to work in |
One thought to consider: I've gotten very into the style of doing this: <Press>
{({ hovered }) => <MotiView animate={{ scale: hovered ? 1.1 : 1 }} />}
</Press> Honestly, I think encouraging this API might be better. Hover interactions are probably best with 2 conditions:
|
I agree with this. That's alright with me, I've started using moti in most of my applications. I think there's probably a way to use this strategy and still make a hover effect themeable from the theme file. |
In addition to the
Pressable
component, it would be nice to style components directly with hover styles.Components should:
API
There are two options I see here:
Or
I think the second one perhaps looks prettier...but the first one might be more scalable. This would ideally let us have hover styles in our theme's variants, etc.
I have to think about which syntax I prefer. Second seems more satisfying to use. 🤔
TODO
mapPropsToStyledComponent
/css
functions. This should probably be handled similar toresponsiveSSRStyles
. Ideally we can think of a method of doing this that would also support other pseudo elements, such as focus, in the future.pseudo
tag on NPM.sx
prop.All the boilerplate stateful hover logic is complete. All that's left is to construct the
hoverStyles
to pass whenisHovered
istrue
.Notes
I am leaning towards not supporting responsive arrays for hover styles at first. It's just going to get really messy with the current implementation. Maybe once it all works, we can add support for this again.