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

[Bug] Properties not recognized by eslint-plugin #135

Open
olivierpascal opened this issue Dec 12, 2023 · 24 comments
Open

[Bug] Properties not recognized by eslint-plugin #135

olivierpascal opened this issue Dec 12, 2023 · 24 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@olivierpascal
Copy link

olivierpascal commented Dec 12, 2023

The problem

The outlineOffset property is not recognized by the eslint-plugin.

How to reproduce

With the following code:

import * as stylex from '@stylexjs/stylex';

export const styles = stylex.create({
  container: {
    outlineOffset: '2px', // eslint(@stylexjs/valid-styles) error here
  },
});

I have the following eslint error from @stylex/valid-styles:

outlineOffset value must be one of:
'outlineOffset' is not supported yet. Please use 'outline' instead

outlineOffset: showError(
"'outlineOffset' is not supported yet. Please use 'outline' instead",
),

But outline syntax does not allow an offset:

outline = 
  <'outline-color'>  ||
  <'outline-style'>  ||
  <'outline-width'>

Cf. https://developer.mozilla.org/en-US/docs/Web/CSS/outline

@olivierpascal olivierpascal changed the title outlineOffset is not recognized by eslint-plugin, but cannot be seted otherwise outlineOffset is not recognized by eslint-plugin, but cannot be set otherwise Dec 12, 2023
@nmn nmn added the bug Something isn't working label Dec 12, 2023
@olivierpascal
Copy link
Author

Same for transition:

export const styles = stylex.create({
  container: {
    transition: 'box-shadow 150ms cubic-bezier(0.4, 0, 0.2, 1)', // eslint(@stylexjs/valid-styles) error here
  },
This is not a key that is allowed by stylex eslint(@stylexjs/valid-styles)

@nmn
Copy link
Contributor

nmn commented Dec 13, 2023

NOTE: A few properties are intentionally not allowed by the ESLint plugin even though they will work correctly in the Babel plugin:

  • background
  • transition
  • animation
  • border, border[Direction].
  • flex

This decision was made for a few reasons:

  1. To encourage better styling practices. We've noticed that a lot of developers don't understand what the various parts of some of these properties are.
  2. To enable smaller CSS. transitions, animations, and background are all properties where many combinations of the same values may be used.
  3. To align a bit with React Native for our future RN implementation.

We are going to be discussing and reconsidering this decision.


Any other missing property is an unknown bug so please report!

Here's a running list so far:

  • all outline longhands
  • placeItems
  • placeContent
  • textDecorationThickness

@olivierpascal
Copy link
Author

Well noted, thanks. Maybe the eslint error message should be more explicit about it, encouraging using individual properties like transitionProperty, transitionDuration and transitionTimingFunction?

@nmn nmn added the good first issue Good for newcomers label Dec 13, 2023
@purohitdheeraj
Copy link
Contributor

I can work on this , looks interesting

@IbraheemHaseeb7
Copy link

Seems fairly straightforward of an issue, can I contribute? Does it require assignee or just submit PR?

@nmn
Copy link
Contributor

nmn commented Dec 13, 2023

There's a few other missing properties in the ESLint rule. You can find them and create a PR to add them.

@xonx4l
Copy link

xonx4l commented Dec 13, 2023

A Pr as per instructions of @nmn to add missing properties.

@olivierpascal
Copy link
Author

const lineHeight = isNumber;

lineHeight could be a string too

@olivierpascal
Copy link
Author

I think transition should be allowed too (#207)

@nmn
Copy link
Contributor

nmn commented Dec 18, 2023

@olivierpascal Yes, as mentioned above, we're discussing adding support for the few shorthands we manually disallowed.

@olivierpascal
Copy link
Author

Css properties placeContent and placeItems are not supported. I guess that you are also discouraging shorthands? I am a big boy and I know what I am doing, can I use them please? ^^

@nmn
Copy link
Contributor

nmn commented Dec 19, 2023

@olivierpascal I'll add them to the list of missing properties in the ESLint plugin that should be allowed. placeContent and placeItems are not intentionally disallowed.

@necolas
Copy link
Contributor

necolas commented Dec 19, 2023

transition won't be allowed becuase...

This decision was made for a few reasons:

One of the other reasons not mentioned is that we want to have consistent merging rules for properties (like React Native), and that doesn't mix with allowing shorthands that can set values for several different properties (which also has an impact on the usefulness of their Types).

outline shouldn't be allowed in source code, and if there's a browser compat issue then the compiler can convert to outline in the output.

flex should be allowed, but only with support for a single numeric value >=0 (e.g., flex: 1), as is done in React Native (excluding the -1 value allowed in RN)

@nmn nmn changed the title outlineOffset is not recognized by eslint-plugin, but cannot be set otherwise outlineOffset and other properties not recognized by eslint-plugin Dec 25, 2023
@facebook facebook deleted a comment from steida Dec 26, 2023
@facebook facebook deleted a comment from steida Dec 26, 2023
@nmn nmn closed this as completed Dec 29, 2023
@steida
Copy link

steida commented Jan 2, 2024

Both scrollbarWidth and "::-webkit-scrollbar"

scrollbarWidth: "none",
"::-webkit-scrollbar": {
display: "none",
},

@steida
Copy link

steida commented Jan 2, 2024

scrollSnapStop

@nmn nmn reopened this Jan 3, 2024
@nmn nmn changed the title outlineOffset and other properties not recognized by eslint-plugin [Bug] Properties not recognized by eslint-plugin Jan 3, 2024
@Daniel15
Copy link
Member

Any other missing property is an unknown bug so please report!

@nmn I posted about this on Workplace too, but we should probably add zoom. It has some caveats but can be more useful than transform: scale(...) because it also affects the layout size of the element. The CSS WG recognises the value it provides and are working on standardising it, and today it works in all major browsers, except Firefox where it's gated by an about:config setting.

@steida
Copy link

steida commented Jan 25, 2024

@nmn This is perfect valid CSS.

display: "-webkit-box",
WebkitBoxOrient: "vertical",
WebkitLineClamp: 2,

@nmn
Copy link
Contributor

nmn commented Jan 26, 2024

Support for all the properties mentioned in this issue were added in v0.5.0 which was just released.

@nmn nmn closed this as completed Jan 26, 2024
@steida
Copy link

steida commented Jan 29, 2024

@nmn lineHeight string is still not supported

 Type 'StyleXClassNameFor<"lineHeight", string>' is not assignable to type '{ _opaque: unique symbol; _key: "lineHeight"; _value: Readonly<number | "initial" | "inherit" | "unset" | null | undefined>; }'.ts(2322)```

@nmn
Copy link
Contributor

nmn commented Feb 1, 2024

@steida Thanks for continuing to catch these issues. Found and updated a bunch of properties in #406 just now.

if you find any additional issues, keep posting them here. I'll see them and fix them.

@nikeee
Copy link
Contributor

nikeee commented Oct 1, 2024

fieldSizing

@nmn
Copy link
Contributor

nmn commented Oct 1, 2024

@nikeee This is a newer property and not something we've intentionally discouraged. Added support for it in #706

@nikeee
Copy link
Contributor

nikeee commented Oct 22, 2024

interpolateSize.

If that's open for grabs, I'd file a PR.

@nmn
Copy link
Contributor

nmn commented Oct 25, 2024

@nikeee go for it.

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

Successfully merging a pull request may close this issue.

9 participants