Skip to content

Usage of title via prop drilling not detected #467

Open
Open
Usage of title via prop drilling not detected#467
Assignees
kendallgassner

Description

@theerebuss
theerebuss
opened on Jul 19, 2023

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

Activity

  1. theerebuss
    assigned
    kendallgassner
    on Jul 19, 2023

  2. kendallgassner commented on Jul 20, 2023

    @kendallgassner
    kendallgassner
    on Jul 20, 2023
    Contributor

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

  3. theerebuss commented on Jul 21, 2023

    @theerebuss
    theerebuss
    on Jul 21, 2023
    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.

  4. kendallgassner commented on Jul 21, 2023

    @kendallgassner
    kendallgassner
    on Jul 21, 2023
    Contributor

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

  5. TylerJDev commented on Jul 27, 2023

    @TylerJDev
    TylerJDev
    on Jul 27, 2023

    @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?

  6. theerebuss commented on Jul 27, 2023

    @theerebuss
    theerebuss
    on Jul 27, 2023
    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.

  7. kendallgassner commented on Jul 27, 2023

    @kendallgassner
    kendallgassner
    on Jul 27, 2023
    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?

  8. theerebuss commented on Jul 28, 2023

    @theerebuss
    theerebuss
    on Jul 28, 2023
    Author

    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.

  9. kendallgassner commented on Jul 28, 2023

    @kendallgassner
    kendallgassner
    on Jul 28, 2023
    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?

  10. theerebuss commented on Jul 31, 2023

    @theerebuss
    theerebuss
    on Jul 31, 2023
    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

  11. TylerJDev commented on Jul 31, 2023

    @TylerJDev
    TylerJDev
    on Jul 31, 2023

    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.

  12. kendallgassner commented on Jul 31, 2023

    @kendallgassner
    kendallgassner
    on Jul 31, 2023
    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.

  13. TylerJDev commented on Aug 1, 2023

    @TylerJDev
    TylerJDev
    on Aug 1, 2023

    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?

  14. kendallgassner commented on Aug 8, 2023

    @kendallgassner
    kendallgassner
    on Aug 8, 2023
    Contributor

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

  15. khiga8 commented on Mar 6, 2024

    @khiga8
    khiga8
    on Mar 6, 2024
    Contributor

    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? :)

  16. byt3n33dl3 commented on Oct 14, 2024

    @byt3n33dl3
    byt3n33dl3
    on Oct 14, 2024

    Agreed, maybe another approach 🧛🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions