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

Suggestion for more robust component settings #863

Open
jrdn91 opened this issue Jun 28, 2022 · 8 comments
Open

Suggestion for more robust component settings #863

jrdn91 opened this issue Jun 28, 2022 · 8 comments

Comments

@jrdn91
Copy link

jrdn91 commented Jun 28, 2022

The custom components update is great! I'm already gaining a ton of benefit by slowly adding the custom components in my project to the settings and getting all the eslint hinting!

I would love to be able to be more specific with how a component behaves via it's props, and what the eslint plugin understands that to mean.

As a use case, we use Material UI for our projects, one of the available components is Typography which handles all text in the app, paragraphs and headings, etc.

Currently the way the custom components settings are setup I would need a separate component called Heading that renders h1, h2, etc. and then Paragraph to render paragraphs and other text elements.

It would be great if I could tell the plugin that Typography elements with variant="h1|h2|h3|h4|h5|h5 should be treated as headings and other Typography elements can be treated as paragraphs to get the other helpful rules such as empty heading to trigger but only when the plugin knows it should be considered a header via that variant prop on the custom component.

This could look something like

settings: {
  "jsx-a11y": {
    components: {
      Box: "div",
      Typography: [
        {
          props: {
            variant: "h1",
          },
          component: "h1"
        },
        {
          props: {
            variant: "h2",
          },
          component: "h2"
        },
        ...
      ]
    },
  },
},

In this way I can specify props and what values I expect and tell the plugin what it should consider those prop combinations to mean in terms of the semantic component it returns or acts like

@khiga8
Copy link
Contributor

khiga8 commented Jul 28, 2022

I'd love to see something like this too! We use Primer React where some components support an as prop so it'd be great if we can map a prop value to an element type.

I don't think we've yet seen the need to support mapping of combinations of props to an element type. I worry this could potentially add complexity and conflicts but maybe it's not as complex as I'm thinking.

If we only want to support a mapping of a single prop type, the following simplified format could work too and could be implemented relatively easily. We've been experimenting with a format like this in one of our custom plugins:

settings: {
  "jsx-a11y": {
    components: {
      Box: "div",
      Typography: {
        props: { variant: { undefined: "p", "p": "p", "h1":"h1", "h2":"h2" } } 
      }
    }
  }
}

This says:

  • <Box> is always treated as div. (same as now)
  • <Typography> is treated as p since variant is undefined and we map variant not being defined to to p.
  • <Typography variant='p'> is treated as p
  • <Typography variant='h1'> is treated as h1
  • <Typography variant='span'> is treated as raw type since variant is set but the value isn't mapped to anything in the configuration.

This wouldn't be very extensible though if there is a need to support combinations of props.

Happy to keep discussing and try to get a PR going if we land on a format!

@ljharb
Copy link
Member

ljharb commented Aug 9, 2022

Can you elaborate on which rules you specifically want to target with these granular settings?

@khiga8
Copy link
Contributor

khiga8 commented Aug 9, 2022

@ljharb I don't believe there's currently an option to configure enable component mappings on a per rule basis.
I'm thinking of this more as an enhancement of the existing component mapping setting.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2022

@khiga8 that's not what i meant. i mean, which specific rules do you hope are impacted by this global setting?

@jrdn91
Copy link
Author

jrdn91 commented Aug 12, 2022

I'm not sure what rules are impacted by heading type elements but that's my specific use case since MUI uses variants for their headings and not separate components for headings, paragraphs, etc.

@khiga8
Copy link
Contributor

khiga8 commented Aug 17, 2022

Same as @jrdn91, I don't have a list of rules in mind. But take for instance, heading-has-content rule which currently does accept components like Heading. It'd be great if we could have it consider something like <Typography variant='h1'>, <Typography variant='h2'>, whether it be on a global level or a rule-level. Another example is, anchor-ambiguous-text. We have a component that can render as an anchor or a button.

@khiga8
Copy link
Contributor

khiga8 commented Aug 25, 2022

Another rule that comes to mind is role-supports-aria-props. Our team would benefit a lot from being able to discourage people from using aria-label on <Text as='p'> for instance.

@milofultz
Copy link

I'm currently working on some components that could benefit from some mappings of a component to a given role and an element with that implicit role does not exist. Most notably, we are working on a tablist component and want to map it to an element with a role of tablist. I went through the issues and don't see an issue about something like this, but was wondering if this has been discussed previously.

I can see a situation that could use an implementation similar to the getElementType utility. When queried for the element type, the settings are queried and pull the associated property in the components object if it exists. Could something similar be done in the getExplicitRole or getImplicitRole utilities?

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

4 participants