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

Configuring plugin to work with polymorphic "as" prop components #923

Open
stephenkoo opened this issue Feb 22, 2023 · 6 comments
Open

Configuring plugin to work with polymorphic "as" prop components #923

stephenkoo opened this issue Feb 22, 2023 · 6 comments

Comments

@stephenkoo
Copy link

Having trouble applying this plugin to polymorphic components that take an as or component prop which determines the html element the component will be rendered as.

For instance the commonly used Box component pattern like the one in Material UI:

<Box component="button">This is a button</Box>
<Box component="h1">This is a H1 element</Box>

The WordPress Gutenberg team seem to have the same issue.

@ljharb
Copy link
Member

ljharb commented Feb 22, 2023

It's unfortunate after decades of folks realizing polymorphism is something to be avoided, that polymorphic patterns are still being used - but that said, it seems like a problem worth solving if people are insisting on doing it anyway.

What schema would you suggest?

@stephenkoo
Copy link
Author

stephenkoo commented Feb 22, 2023

@ljharb I'm keen to learn a better way besides polymorphic components to address the use case of easily passing style-related props to any html element, if you're aware of better practice.

For example:

<Box width={[1, 1 / 2]} p={4} mb={3} bg="tomato">
  This is a Styled System box
</Box>
<Box bg='tomato' w='100%' p={4} color='white'>
  This is a Chakra UIBox
</Box>
  • A Text component that takes a color prop of primary, secondary, success, etc. (or intent of title, heading, subheading, body, etc.) that determines its look whilst being able to be rendered as h1, h2, h2 (an example here)
<Text as="h1" intent="body">This looks like regular text but is a h1 element</Text>
<Text as="p" intent="title">This looks like a large title but is a p element</Text>
<Text as="button" intent="subheading" onClick={handleClick}>This looks like a subheading title but is a p but is a button</Text>

@ljharb
Copy link
Member

ljharb commented Feb 22, 2023

In my experience each thing has a manually created wrapper component for it, with explicit props to control the appearance - not actually allowing anyone to pass specific colors or sizes etc, but instead, providing an explicit list of named configurations.

That said, let's keep the issue on topic :-) what schema do you suggest for a component that has a specific property to govern its "kind"?

@stephenkoo
Copy link
Author

stephenkoo commented Feb 22, 2023

Thanks. If what you mean by schema is the JSON schema for amending the plugin, how's this:

{
  "settings": {
    "jsx-a11y": {
      "polymorphicComponents": [
        {
          "components": ["Text", "AnotherPolymorphicComponent"],
          "prop": "as",
          "values": [
            "html", // "html" is a special value that allows all html elements
            {
              "value": "fraggieBoy", // <Text as="fraggieBoy" /> is linted as a react fragment
              "type": "fragment"
            },
            {
              "value": "buttonOne", // if <Text as="buttonOne" /> is used, it will be linted as a button
              "type": "button"
            }
          ],
          "default": "div" // if no prop is provided, linted as a div. if no default is provided, a11y does not lint this component until prop is provided.
        },
        {
          "components": ["Box"],
          "prop": "component",
          // if no "values" is provided, all html elements are allowed, fragment is not allowed
        },
        {
          "components": ["DifferentPolymorphicComponent"],
          "prop": "renderAs",
          "values": ["html", "fragment"] // <DifferentPolymorphicComponent renderAs="fragment" /> is linted as a react fragment
        }
      ]
    }
  }
}

The biggest limitation which I've not good solution for is how does the linter work with conditional as props, e.g.

const App = () => {
  const isClickable = true;

  return (
    <Box as={isClickable ? "button" : "div"}>I may be a clickable button. How do I lint this?</Box>
  )
}

Would it be possible for the linter to handle this if we passed the appropriate html attributes conditionally? E.g.

const App = () => {
  const isClickable = true;

  return (
    <Box
      ...{isClickable ? {
        as: "button",
        onClick: () => { console.log("clicked") },
       ...otherButtonProps
      } : {
        as: "div"
      }
    >
      I may be a clickable button. How do I lint this?
    </Box>
  )
}

If it's the implementation detail, I'm probably going to have to take a deeper look into the a11y plugin.

@khiga8
Copy link
Contributor

khiga8 commented Mar 9, 2023

Wanted to jump in and +1, this would be great!! This also came up in: #863.

The biggest limitation which I've not good solution for is how does the linter work with conditional as props

I agree. This might be a bit more complex to figure out, and I think it's worth kicking off support with just literal mapping and iterate on that. I believe we could get something going pretty quickly with just updating: getElementType.

@khiga8
Copy link
Contributor

khiga8 commented Mar 13, 2023

I don't have super strong opinions, but just some initial thoughts/questions on the proposed schema:

is linted as a react fragment

Should this linter care about if something maps to a React fragment? Based on the existing rules, it seems like the linter only cares if something maps to an HTML element it recognizes, so maybe for now, we don't need to worry about explicitly mapping fragments in the configs.

"html" is a special value that allows all html elements

Given this is a "special value", could it be more appropriate as an explicit key?

          "components": ["Text", "AnotherPolymorphicComponent"],
          "prop": "as",
          "html": true,
          "values": [{"button1": "button"}]

"html" is a special value that allows all html elements

I am assuming that if this linter comes across something like: <SomeComponent as="some-custom-element"> where the tag corresponds to a custom element that isn't recognized, this would just be ignored unless there's a mapping provided in "values".

"components": ["Text", "AnotherPolymorphicComponent"],

I like that this makes it convenient for the consumer to set multiple components to the same config as I'd imagine a lot of components will have the same exact config, but I also find it a bit less-readable than if we used the component as a key like in components. Maybe that's not a big deal but just a thought. As the configs can get complex, it could be nice to provide the consumer with validation to ensure the configs aren't in a weird state like a component appearing multiple times in the configs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants