facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.4k stars 309 forks source link

Styling based on parent state #373

Closed martingrzzler closed 9 months ago

martingrzzler commented 9 months ago

Describe the feature request

I have a simple Tooltip component that uses the Tailwind group class to change the styles of a child div when the group is hovered. With StyleX embracing encapsulation there seems to be no easy way of doing this other than using useState with mouseenter and mouseleave events. Is that how one is supposed to handle these scenarios?

import { borders } from '@stylexjs/open-props/lib/borders.stylex'
import { fonts } from '@stylexjs/open-props/lib/fonts.stylex'
import { sizes } from '@stylexjs/open-props/lib/sizes.stylex'
import * as stylex from '@stylexjs/stylex'
import { useState } from 'react'

export default function Tooltip({ children, tooltip }: { children: React.ReactNode; tooltip: React.ReactNode }) {
  const [hovering, setHovering] = useState(false)
  return (
    <div
      onMouseEnter={() => setHovering(true)}
      onMouseLeave={() => setHovering(false)}
      className="flex group relative items-center justify-center"
    >
      {children}
      <div
        {...stylex.props(styles.tooltip, hovering ? styles.active : null)}
        // className="absolute bottom-full mb-2 left-1/2 transform -translate-x-1/2 px-2 py-1 text-sm text-white bg-black rounded-md z-10 opacity-0 group-hover:opacity-100"
      >
        {tooltip}
      </div>
    </div>
  )
}

const styles = stylex.create({
  tooltip: {
    opacity: 0,
    position: 'absolute',
    bottom: '100%',
    left: '50%',
    transform: 'translateX(-50%)',
    padding: `${sizes.spacing1} ${sizes.spacing2}`,
    fontSize: fonts.size1,
    zIndex: 10,
    backgroundColor: 'black',
    color: 'white',
    borderRadius: borders.radius2,
    marginBottom: sizes.spacing1,
  },
  active: {
    opacity: 1,
  },
})
nmn commented 9 months ago

Something like this is planned for the future.

As of today, it is possible to use variables to workaround this issue without resorting to JS event handlers. (See Card.tsx under the NextJS example.)

However, the CSS hover state is generally error-prone on touch-screens and seems to get stuck. It might be better to use a custom hook like useHover from react-aria-components if you want to handle hover correctly for both mouse and keyboard. See: https://react-spectrum.adobe.com/blog/building-a-button-part-2.html

martingrzzler commented 9 months ago

I still don't see how to to do something like the following without JS event handlers.

#one {
  background-color: red;
}

#two {
  background-color: green;
  opacity: 0;
}

#one:hover ~ #two {
  opacity: 1;
}

div {
  width: 50px;
  height: 50px;
}

<div id="one"></div>
<div id="two"></div>
bonttimo commented 9 months ago

@martingrzzler Is this what you are after CodeSandBox demo?

With this, you can show a child element when you hover over a parent.

const styles = stylex.create({
    tooltip: {
        ":hover > .tooltip": {
            transform: "translateY(-100%)",
            opacity: "1",
        },
    },
    popup:{
       opacity: "0",
       transform: "translateY(0)",
    }
});

And in your HTML you add the tooltip class.

<div {...stylex.props(styles.tooltip)}>
    <p>This is a tooltip</p>
    <div className={`tooltip ${stylex.props(styles.popup).className}`}>StyleX is great!</div>
</div>
martingrzzler commented 9 months ago

@bonttimo Yes exactly! Is that somewhere to be found in the Docs? While it does feel hacky, it works.

bonttimo commented 9 months ago

@martingrzzler No, it's not in the Docs. I encountered the same issue and, while experimenting, I noticed that this approach worked. It feels a bit hacky, but then it's proper CSS syntax. Hopefully, it will keep on working in the future. 😄

I observed that it works when you do:

<div className={`tooltip ${stylex.props(styles.popup).className}`}>StyleX is great!</div>

But then this does not work:

<div className={`tooltip ${stylex(styles.popup)}`}>StyleX is great!</div>
martingrzzler commented 9 months ago

@bonttimo Nice! One problem I see with this is that we now have a class name tooltip that could be used anywhere, leading to bugs and unexpected behaviour. Given the the encapsulation part of the docs this should not even be possible.

.className > *
.className ~ *
.className:hover > div:first-child

All of these patterns, while powerful, make styles fragile and less predictable. Applying class names on one element can affect a completely different element.

necolas commented 9 months ago

Well, none of those selectors are supposed to be supported, especially allowing arbitrary class names in the key

<div className={tooltip ${stylex.props(styles.popup).className}}>StyleX is great!</div>

This is also very hacky and shouldn't be done.

bonttimo commented 9 months ago

@martingrzzler That's true. However, with this approach, you wouldn't be able to style a simple navigation with a dropdown without using JavaScript, which seems odd to me. Also, the documentation mentions "styles at a distance" but in my opinion, this approach leads to "styles from an even larger distance." because people will create one-off CSS files for basic hover effect styling.

necolas commented 9 months ago

We've styled several huge apps at Meta with this sytem and that hasn't been the case.

nmn commented 9 months ago

@bonttimo Using the .tooltip class is styling at a distance. The styles for that class are being set in a place that is not where the class is being applied.

This hack is not a supported pattern and will break in the future. Sibling selectors are currently unsupported in StyleX.

Frankyfrankly commented 9 months ago

@necolas So how did you deal with case like this in Meta using stylex system only?

.parent {
  background-color: red;
}
.parent .child {
  background-color: blue;
}
.parent:hover .child {
  background-color: green;
}

@nmn do you have any alternative to that hack or to the mouse event detect hover approach?

nmn commented 9 months ago

@Frankyfrankly As mentioned, the CSS :hover is not good for touchscreens so we use a JS-based approach for it.

philipfeldmann commented 8 months ago

@nmn Does that mean that we cannot use stylex ever to override styles from an existing UI component library? Most libraries, especially older ones, only expose a single className and expect you to overwrite styles using styling at a distance.

necolas commented 8 months ago

Yes that's pretty much correct. StyleX is a system for encapsulating styles; libraries that allow arbitary styling of their sub-tree is the exact pattern StyleX is designed to avoid.

nmn commented 8 months ago

@philipfeldmann StyleX is not a replacement for CSS, it's a component styling system. If you need selectors, do not use StyleX.

zaydek commented 8 months ago

Just wanted to jump into the conversation. I've tried a couple of approaches. I initially tried just using event handlers but it felt pretty unwieldy to me and there were edge cases where if the cursor jumped outside of the browser the pointerup event never fired meaning I had a tooltip that forgot to close itself. I'm sure if I put a little more effort into that approach I could make it work but it wasn't my favorite.

One of the reasons I struggle with event handlers for this is because let's say I have a hook called useHover. If I have a map and I am building a lot of components in JSX, I have to extract the map loop to a separate component because of the rules of hooks. Alternatively I would need to make a useHoverByIndex that accepts an index or a key but I'm not just sure about approach. But I'm open minded. Any advice on how that user story should look in StyleX @nmn?

Now I'm using CSS variables to compose group variants, and while it requires a little coordination it feels fairly reasonable:

const cssVarTextAltOpacity = "--text-alt-opacity"

const dropdownStyles = create({
  // ...
  listItem: {
    [cssVarTextAltOpacity]: {
      default: "0.375",
      ":is(:hover, :focus-visible)": "1",
    },
    alignItems: "center",
    backgroundColor: {
      ":is(:hover, :focus-visible)": "#0c8ce9", // TODO: Extract to CSS variable
    },
    display: "flex",
    gap: 8,
    paddingBlock: 2, // 18 + 2 * 2 = 24
    paddingInline: 16,
  },
  // ...
  listItemTextAlt: {
    opacity: `var(${cssVarTextAltOpacity})`,
  },
})

The nuance with not using event handlers is that the code isn't portable to RN code bases (not a pressing concern for me but a valid one).

I'd love to see how people are implementing the useHover story in a reliable way so that edge cases like if the browser becomes inactive or the pointer leaves the browser window, things work and don't feel broken.

Edit: Here's an example of my use case for why I want to use group variant. Notice the text goes full white when the list item is hovered.

https://github.com/facebook/stylex/assets/58870766/bd0be567-78a9-4902-8c60-36314f425484

zaydek commented 8 months ago

Quick follow up: I just found that even CSS doesn't know when the cursor leaves the browser window (by pressing command-tab the browser loses focus and the cursor can move freely without triggering the :hover to release). Here's the simple CodePen: https://codepen.io/zaydek/pen/dyrBPRv.

This is something I run into a lot in Figma plugins because your window is a 400px x 400px rectangle and the cursor can 'jump' out of the plugin window for many reasons. I didn't realize CSS has more or less the same problems though.

https://github.com/facebook/stylex/assets/58870766/7a258722-7d7f-416b-9937-c63f5a9bb36b

Is there a canonical implementation for useHover that is advised for composing group variants? I know it's probably simple but I'm curious if anyone has a solution that is battle hardened for their use case.

necolas commented 8 months ago

There's one in the react-native-web source code. It's a little outdated as today we'd only need to use PointerEvent, but it should get you started

nmn commented 8 months ago

@zaydek There's an example of using CSS variables to do styling based on parent state here. defineVars can be used to define a variables instead of using a string, but it does need to be in a separate file.

react-aria-components also has an example of a useHover hook.

zaydek commented 8 months ago

Thanks guys. This clears up some confusion I had. I'll surface both links here in case anyone is interested:

Spectrum: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/useHover.ts RNW: https://github.com/necolas/react-native-web/blob/5f9c32eba0f840bde7b462a57d0748358ff866f6/packages/react-native-web/src/modules/useHover/index.js#L61

@nmn Besides the constraint of requiring a separate file this looks pretty good:

  link: {
    // ...
    [tokens.arrowTransform]: {
      default: 'translateX(0)',
      ':hover': 'translateX(4px)',
    },
    // ...
  },
  span: {
    // ...
    transform: tokens.arrowTransform,
  },

It's a small thing but it's nice you don't have to interpolate the variable in var(${...}) here. In any case, it looks like I should stick with CSS variables. Using useHover or equivalent seems like too much machinery for my use case and too error prone if I try to rewrite it. I just wanted to confirm/disconfirm if I was going about things in a reasonable way with WRT to CSS variables. Thanks!

necolas commented 8 months ago

IIRC the Adobe hook is based on an even older, experimental event hook I wrote while on the React core team.

I wrote another hover hook for the project that RSD was extracted from. That one supports control over bubbling too. We might open source those hooks in a separate package once React Native supports EventTarget on host elements

zaydek commented 8 months ago

IIRC the Adobe hook is based on an even older, experimental event hook I wrote while on the React core team.

I wrote another hover hook for the project that RSD was extracted from. That one supports control over bubbling too. We might open source those hooks in a separate package once React Native supports EventTarget on host elements

If you do, the use cases I would want to compose are :hover, :focus-visible. The nuance is that if one of these gets exposed as an API people will ask for more: useHover, useFocus, useFocusVisible, useFocusWithin (which seems to be more or less the path that Spectrum took). As a consumer of this, the CSS variable API is a lot more appealing to me because it has no runtime implications and is a lot more familiar to strictly web devs. But for the universal use case, such as users using RSD in the future, it could make a lot of sense to have those provided.

@nmn I wonder if it would make sense to expose a utility defineVar function or like cssVar function that can be used in JS (not requiring .stylex.ts) so that users can generate colocated CSS variables on the fly and it handles the --var-name, var(--var-name) much the same as defineVars already does.

For example:

const transform = cssVar()

const styles = stylex.create({
  foo: {
    [transform]: {
      default: "translateY(0px)",
      ":is(:hover, :focus-visible)": "translateY(2px)",
    },
  }
  bar: {
    transform,
  },
})

It's a small quality of life thing but if the API did exist, it may help address users who want to style using 'side effects' in a safer way with less boilerplate. But it could also lead to confusion if users try to start manipulating transform in their JS, expecting it to give --<HASH> when it actually gives var(--<HASH>).

necolas commented 8 months ago

Yeah we wrote hooks for all of those, which is where the Adobe team got the idea from. I agree that for the most part having the CSS capability is best for simple use cases, although hover is fundamentally not great for touch experiences on web. However, having hooks for more complex use cases related to supporting reactive changes to the DOM tree is also handy. Hopefully there is a path for React Native to support the CSS syntax natively, but in the mean time we can probably polyfill more of the CSS syntax for RN in the same way we've polyfilled :hover. But extending that to parent-child styling is unlikely

nmn commented 8 months ago

I wonder if it would make sense to expose a utility defineVar function or like cssVar function that can be used in JS (not requiring .stylex.ts)

We will be looking into allowing defineVars within regular files as long it is only used locally. We're purposely waiting for such changes as we want to take the time to learn the ramifications of our current decision.

":is(:hover, :focus-visible)"

Please don't combine :hover and :focus-visible like that. I know it's repetitive, but keep them separate. It results in smaller CSS. As the compiler gets stricter, this pattern will not be allowed in the future.


although hover is fundamentally not great for touch experiences on web

Here's adobe article explaining the issues with hover on touch screens and why there is no perfect solution without JS:

https://react-spectrum.adobe.com/blog/building-a-button-part-2.html

oliviertassinari commented 7 months ago

As mentioned, the CSS :hover is not good for touchscreens so we use a JS-based approach for it.

@nmn I got funny results with this testing on different phones: https://github.com/mui/mui-x/issues/10039#issuecomment-1690755359. I'm curious it seems that '@media (hover: hover) and (pointer: fine)': is a good enough solution for the problem. It's used by GitHub: https://github.com/primer/react/blob/0f76bf1c4c08ed7517a3c01d42b7d8a256b724f0/packages/react/src/deprecated/ActionList/Item.tsx#L195. Why no go with this?

nmn commented 7 months ago

I'm curious it seems that '@media (hover: hover) and (pointer: fine)': is a good enough solution for the problem.

Sadly it's not a good enough solution when you take into account devices where both a touchscreen and a mouse pointer exist. A large majority of new Windows laptops have touchscreens and iPads exist, so this is a relatively common occurrence.