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

StyleX adds logical properties classes when it should not #796

Open
zaydek opened this issue Dec 5, 2024 · 3 comments
Open

StyleX adds logical properties classes when it should not #796

zaydek opened this issue Dec 5, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@zaydek
Copy link

zaydek commented Dec 5, 2024

Describe the issue

Related to #752.

This was very confusing for me to discover. I believe StyleX has a bug when replacing logical properties.

See the following code for example:

import * as stylex from "@stylexjs/stylex";
import { CSSPropertiesWithExtras } from "@stylexjs/stylex/lib/StyleXTypes";

const styles = stylex.create({
  idea: (args: { width?: CSSPropertiesWithExtras["width"] }) => ({
    width: args.width,
  }),
});

export function Idea() {
  return (
    <div {...stylex.props(styles.idea({ width: 64 }))}>
      <div {...stylex.props(styles.idea({}))}>Hello</div>
    </div>
  );
}

Output (this is correct)::

<div class="idea__styles.idea width-x1bl4301" style="--width: 64px;">
  <div class="idea__styles.idea">Hello</div>
</div>

Now see what happens when we use inline-size instead of width:

import * as stylex from "@stylexjs/stylex";
import { CSSPropertiesWithExtras } from "@stylexjs/stylex/lib/StyleXTypes";

const styles = stylex.create({
  idea: (args: { inlineSize?: CSSPropertiesWithExtras["inlineSize"] }) => ({
    inlineSize: args.inlineSize,
  }),
});

export function Idea() {
  return (
    <div {...stylex.props(styles.idea({ inlineSize: 64 }))}>
      <div {...stylex.props(styles.idea({}))}>Hello</div>
    </div>
  );
}

Output (I believe this is incorrect):

<div class="idea__styles.idea width-x1avlqir" style="--inlineSize: 64px;">
  <div class="idea__styles.idea width-x1avlqir">Hello</div>
                                ^^^^^^^^^^^^^^
</div>

When using logical properties, it looks like StyleX is adding the class when it should not, because in this case inlineSize is undefined and therefore the class should be omitted altogether. This was causing me a lot of problems because it took me a while to figure out it seems to be a side effect of logical properties and not all properties.

I'll try disabling styleResolution: 'legacy-expand-shorthands' and see if this problem goes away on its own.

Expected behavior

See above

Steps to reproduce

See above

Test case

No response

Additional comments

No response

@zaydek
Copy link
Author

zaydek commented Dec 5, 2024

Follow up: It doesn't look like the Vite plugin I'm using vite-plugin-stylex touches styleResolution which would lead me to believe it is already set to the default setting 'application-order' as described by the docs. So this seems to be potentially an isolated error related to logical properties and not styleResolution, I think.

@nmn
Copy link
Contributor

nmn commented Dec 5, 2024

This is a known issue. This was originally done to intentionally disallow vertical writing modes and to normalize style properties that are safe to do so for LTR and RTL "horizontal" layouts. I.e. Unless you're using vertical writing modes, inline-size and width are equivalent.

The rationale for this decision was to minimize the total size of the CSS created and maximize CSS rule re-usability.

Another reason was the fact that it is possible to polyfill horizonal logical styles but not vertical writing modes on browsers that don't support logical properties natively.


We will soon fix this problem and officially start supporting vertical writing modes. The support for logical properties in browsers is widespread and we don't need to worry about polyfills anymore.

@zaydek
Copy link
Author

zaydek commented Dec 6, 2024

OK thanks. Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants