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

Usage of title via prop drilling not detected #467

Open
andrefcdias opened this issue Jul 19, 2023 · 15 comments
Open

Usage of title via prop drilling not detected #467

andrefcdias opened this issue Jul 19, 2023 · 15 comments
Assignees

Comments

@andrefcdias
Copy link

Recently found a couple of cases of title attribute usage (soon to be addressed in https://github.com/github/issues/issues/7066 🙏) that are not detected by the rule.

This seems to be the case when prop drilling title into a component, unsure if because this specific scenario is overriding the type prop or something else.

Root usage of the attribute
Props object with the title prop being spread into a button
Prop type overriding title

@kendallgassner
Copy link
Contributor

This could be challenging because the jst-ast-util function we use to grab the title attribute only checks the direct keys.

@andrefcdias
Copy link
Author

Agreed, maybe another approach could be to throw warning on usage of title directly in Primer components? We'd still have problems detecting prop drilling for regular JSX elements, but it could give another layer of security.

@kendallgassner
Copy link
Contributor

cc. @TylerJDev I wonder if we can just omit the title props from this Primer component?

@TylerJDev
Copy link

@kendallgassner, In this example yeah! I think that the title prop should be removed from IconProps. I'm not entirely sure why it's present here, as it seems to be adding the native title attribute to the Button which doesn't seem correct.. This is what I'm thinking it looks like when rendered:

<button title="Passed text" ...>
    <span>Edit Passed Text</span>
</button>

We'd definitely want to remove it in this instance. I don't think this is tied to Primer though. I'm wondering, is this a shared component?

@andrefcdias
Copy link
Author

This particular instance was already fixed in https://github.com/github/github/pull/281501

SectionHeader is just a shared component used for the metadata sections.

@kendallgassner
Copy link
Contributor

In this particular case I think cleaning up the Props and removing title: string was the right move! Even though title is still available in the React.HTMLAttributes<HTMLButtonElement> prop object I believe teams will feel less inclined to use it since it is no longer brought to their attention.

@andrefcdias do you feel like removing the hardcoded prop from the SectionHeader was enough? Or could you add a warning to the SectionHeader component since it is not a Primer component?

@andrefcdias
Copy link
Author

andrefcdias commented Jul 28, 2023

That and the awareness raised within my team should be more than enough because we own the component in question and the shared component that uses it. My original concern was more about this scenario being a possibility, which can go under the radar as it is not direct title usage.

I also like the idea of either omitting title from Primer components or adding warnings for the usage of it, to detect these harder to lint scenarios, but understand that this could impact the Developer Experience negatively.

@kendallgassner
Copy link
Contributor

@TylerJDev thoughts here?

We have some Primer components that use the prop name title; but the title prop in these components does not get used as a semantic title attribute in the rendered HTML element. It be nice to avoid using this prop name but I don't think we are likely to change these props name since it might require a major version bump?

@andrefcdias
Copy link
Author

We have some Primer components that use the prop name title; but the title prop in these components does not get used as a semantic title attribute in the rendered HTML element.

FYI, I've also seen this happen in some of the shared components

@TylerJDev
Copy link

Definitely agree, I think it'd be beneficial for us to steer away from using HTML attributes as prop names when there's no direct connection. This would involve a breaking change for most components that currently do this, but I'd like to get the idea out there.

I also like the idea of either omitting title from Primer components or adding warnings for the usage of it, to detect these harder to lint scenarios, but understand that this could impact the Developer Experience negatively.

We could have a lint rule specific to Primer components in eslint-plugin-primer-react that warns users against using title. This would only be a temporary stopgap until we change the existing props that utilize HTML attributes for their names. I think the title usage in SectionHeader would still be an issue, as I don't believe we'd be able to determine usage via props passed with spread.

@kendallgassner, I forget if this was discussed already. Do you think this should be something eslint-plugin-primer-react should handle? The only difference from the rule in eslint-plugin-github is that it would now apply to Primer components, but other than that it would be the same. I'm thinking this could work, but might also be redundant.

@kendallgassner
Copy link
Contributor

eslint-plugin-github could only test against semantic HTML components because we didn't want to flag react components that might use the title attribute differently.

If we add the lint rule in eslint-plugin-primer-react we could expand are testing to not only flag semantic components but to also check against the primer specific components -- since we have more control of these components.

Thinking 🤔 though we would need to account for the as prop.

@TylerJDev
Copy link

If we did test Primer specific components, would we want to ignore as usage and still throw a violation regardless of what value it might have? I'm wondering, are there cases where title might be appropriate outside of iframe?

@kendallgassner
Copy link
Contributor

kendallgassner commented Aug 8, 2023

And I can't think of a case where we would want a Primer component with as=iframe except maybe Box.

@khiga8
Copy link
Contributor

khiga8 commented Mar 6, 2024

Definitely agree, I think it'd be beneficial for us to steer away from using HTML attributes as prop names when there's no direct connection. This would involve a breaking change for most components that currently do this, but I'd like to get the idea out there.

Revisiting this discussion... @TylerJDev I 100% agree and would fully support this!

Is there a tracking issue or discussion I could reference? :)

@byt3n33dl3
Copy link

Agreed, maybe another approach 🧛🏻

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

5 participants