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.

Sign up for GitHub

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

Jump to bottom

chore: custom ESLint rule to disable sorting for ref attribute to keep it as last property in JSX to ensure it is up-to-date #7669

Merged
merged 22 commits into from
Nov 2, 2023
Merged

chore: custom ESLint rule to disable sorting for ref attribute to keep it as last property in JSX to ensure it is up-to-date #7669

merged 22 commits into from
Nov 2, 2023

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Sep 3, 2023 •
edited
Loading

Related Issue: #7659

Summary

Custom ESLint rule to provide a more granular prop ordering mechanism by forcing the ref attribute to be kept as the last property so we can ensure it is up-to-date.

Added a test file for the new custom rules: the file references different use cases, which are 1 good and 1 bad usage files.

@Elijbet
chore: custom ESLint rule to disable sorting for ref attribute to kee…
be5b936 
…p it as last property in JSX to ensure it is up-to-date
@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Sep 3, 2023
@alisonailea
Copy link
Contributor

It doesn't look like jsx-eslint/eslint-plugin-react has kept their docs up to date. There is a prop called reservedList available for the eslint react/jsx-sort-props that should allow "ref" to be at the bottom of a multiline list of attributes.

"react/jsx-sort-props": [
      "error",
      {
        reservedList: ["ref"],
      },
 ],

@Elijbet
Copy link
Contributor Author

Elijbet commented Sep 12, 2023

It doesn't look like jsx-eslint/eslint-plugin-react has kept their docs up to date. There is a prop called reservedList available for the eslint react/jsx-sort-props that should allow "ref" to be at the bottom of a multiline list of attributes.

"react/jsx-sort-props": [
      "error",
      {
        reservedList: ["ref"],
      },
 ],

I would rather not use an undocumented prop, it doesn't feel like a good practice.
It's possible they just forgot to document it, but it is also possible that it wasn’t documented intentionally and only exists in the source code because they don’t want it being used for whatever reason. Have we not ever done the latter before?

But I'll defer to @driskull and @jcfranco for perspective.

@alisonailea
Copy link
Contributor

I ran quick test on my local branch before I made the comment. However, after opening a clean branch off of Main and running some additional tests, I don’t think it’s working the way I thought.

That said, I think it would be better to open an issue and PR to eslint-jsx/eslint-react with a "reservedLast" prop (or something) rather than making (and then having to maintain) a new eslint plugin.

@driskull
Copy link
Member

I would rather not use an undocumented prop, it doesn't feel like a good practice.

Agreed.

I ran quick test on my local branch before I made the comment. However, after opening a clean branch off of Main and running some additional tests, I don’t think it’s working the way I thought.

reservedList sounds like its something that isnt allowed to be used?

That said, I think it would be better to open an issue and PR to eslint-jsx/eslint-react with a "reservedLast" prop (or something) rather than making (and then having to maintain) a new eslint plugin.

Agreed. We should open an issue/PR but in the meantime we could do our own thing with a custom plugin. Who knows if they would add or accept the feature we are requesting.

@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Sep 20, 2023
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Sep 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2023

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 1, 2023
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Oct 11, 2023
@Elijbet Elijbet marked this pull request as ready for review October 11, 2023 18:10
@Elijbet Elijbet added the skip visual snapshots Pull requests that do not need visual regression testing. label Oct 11, 2023
driskull
driskull reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Maybe the wording could be cleaned up a little.

packages/eslint-plugin-calcite-components/src/rules/enforce-ref-last-prop.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-calcite-components/src/rules/enforce-ref-last-prop.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-calcite-components/src/rules/enforce-ref-last-prop.ts Outdated Show resolved Hide resolved
packages/eslint-plugin-calcite-components/src/rules/enforce-ref-last-prop.ts Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Oct 20, 2023
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Oct 25, 2023
@Elijbet Elijbet mentioned this pull request Oct 29, 2023
Closed
4 tasks
jcfranco
jcfranco approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️
🕶️😎😎😎😎🕶️😎🕶️🕶️🕶️🕶️🕶️😎🕶️😎😎😎😎🕶️🕶️😎😎😎🕶️🕶️😎😎🕶️🕶️😎🕶️🕶️🕶️😎🕶️😎😎😎😎🕶️😎🕶️
🕶️😎🕶️🕶️😎🕶️😎🕶️🕶️🕶️🕶️🕶️😎🕶️😎🕶️🕶️🕶️🕶️😎🕶️🕶️🕶️🕶️😎🕶️🕶️😎🕶️😎😎🕶️😎😎🕶️😎🕶️🕶️🕶️🕶️😎🕶️
🕶️😎😎😎😎🕶️😎🕶️🕶️😎🕶️🕶️😎🕶️😎😎😎🕶️🕶️🕶️😎😎🕶️🕶️😎🕶️🕶️😎🕶️😎🕶️😎🕶️😎🕶️😎😎😎🕶️🕶️😎🕶️
🕶️😎🕶️🕶️😎🕶️😎🕶️😎🕶️😎🕶️😎🕶️😎🕶️🕶️🕶️🕶️🕶️🕶️🕶️😎🕶️😎🕶️🕶️😎🕶️😎🕶️🕶️🕶️😎🕶️😎🕶️🕶️🕶️🕶️🕶️🕶️
🕶️😎🕶️🕶️😎🕶️🕶️😎🕶️🕶️🕶️😎🕶️🕶️😎😎😎😎🕶️😎😎😎🕶️🕶️🕶️😎😎🕶️🕶️😎🕶️🕶️🕶️😎🕶️😎😎😎😎🕶️😎🕶️
🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️🕶️

packages/calcite-components/.eslintrc.cjs Outdated Show resolved Hide resolved
packages/eslint-plugin-calcite-components/README.md Outdated Show resolved Hide resolved
packages/eslint-plugin-calcite-components/README.md Outdated Show resolved Hide resolved
packages/eslint-plugin-calcite-components/src/rules/enforce-ref-last-prop.ts Outdated Show resolved Hide resolved
...ugin-calcite-components/tests/lib/rules/enforce-ref-last-prop/enforce-ref-last-prop.good.tsx Show resolved Hide resolved
jcfranco
jcfranco approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be good to merge once the last comments are addressed. 🚀

...gin-calcite-components/tests/lib/rules/enforce-ref-last-prop/enforce-ref-last-prop.wrong.tsx
id={`${guid}-element`}
onClick={() => {
/* click! */
}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabIndex should go here to match the good case.

packages/eslint-plugin-calcite-components/README.md Outdated Show resolved Hide resolved
@Elijbet Elijbet merged commit 589d09f into main Nov 2, 2023
11 checks passed
@Elijbet Elijbet deleted the elijbet/7659-custom-eslint-rule-enforce-ref-last-prop branch November 2, 2023 23:09
@github-actions github-actions bot added this to the 2023 November Priorities milestone Nov 2, 2023
@Elijbet Elijbet mentioned this pull request Nov 5, 2023
Merged
jcfranco added a commit that referenced this pull request Nov 8, 2023
@Elijbet @jcfranco
refactor: improve on enforce-ref-last-prop ESLint rule docs (#8129)
142dc20 
**Related Issue:** #7669, #7659

## Summary

Refactor to improve on `enforce-ref-last-prop` ESLint rule docs.

---------

Co-authored-by: JC Franco <[email protected]>
jcfranco added a commit that referenced this pull request Nov 9, 2023
@Elijbet @jcfranco
refactor: improve on enforce-ref-last-prop ESLint rule docs (#8129)
2aeac45 
**Related Issue:** #7669, #7659

## Summary

Refactor to improve on `enforce-ref-last-prop` ESLint rule docs.

---------

Co-authored-by: JC Franco <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull left review comments

@alisonailea alisonailea alisonailea approved these changes

@jcfranco jcfranco jcfranco approved these changes

@benelan benelan Awaiting requested review from benelan

@geospatialem geospatialem Awaiting requested review from geospatialem

Assignees

@Elijbet Elijbet

Labels
chore Issues with changes that don't modify src or test files. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Milestone
2023 November Priorities
Development

Successfully merging this pull request may close these issues.
4 participants
@Elijbet @alisonailea @driskull @jcfranco