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

fix(lint): replace non-existent qe-id with data-qe-id #543

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Feb 14, 2024

Motivation

  • lint was erroring out: Unknown property 'qe-id' found react/no-unknown-property

I'm not sure why it didn't pop up during #509 though... It's also only popping up on certain PRs for some reason, but not all of them. Like it pops up on #534 but not #536, despite neither of them changing any source code EDIT: See below comment, this happens on newer versions of ESLint

Modifications

  • qe-id -> data-qe-id
    • data-* attributes are the standardized way of persisting custom data to HTML
      • qe-id was used for testing purposes in this repo
        • this type of test is no longer standard, particularly in React, but for now I just renamed it to get lint passing

Verification

lint passes now

- `lint` was erroring out: `Unknown property 'qe-id' found  react/no-unknown-property`

- [`data-*` attributes](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/data-*) are the standardized way of persisting custom data to HTML
  - `qe-id` was used for testing purposes in this repo
    - this type of test is no longer standard, particularly in React, but for now I just renamed it to get `lint` passing

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Testing locally, it looks like this change breaks Argo CD's application list autocomplete. Here it is after reverting to HEAD.
image

@agilgur5
Copy link
Author

agilgur5 commented Feb 27, 2024

Well that's surprising, qe-id isn't a valid attribute on input elements (hence the lint error) and is a convention for tests... It shouldn't be used for any actual behavior at all 🤔

@agilgur5
Copy link
Author

agilgur5 commented Mar 5, 2024

Testing locally, it looks like this change breaks Argo CD's application list autocomplete.

Even more confusingly, CD's Application List uses the Autocomplete component, which did not change here (not the AutocompleteField that changed here)

This seems unrelated to me as such, was there another change to argo-ui that potentially broke it?

This comment was marked as resolved.

@github-actions github-actions bot added the problem/stale This has not had a response in some time label May 5, 2024
@agilgur5 agilgur5 removed the problem/stale This has not had a response in some time label May 5, 2024
@agilgur5 agilgur5 marked this pull request as draft May 12, 2024 03:27
@agilgur5
Copy link
Author

Like it pops up on #534 but not #536, despite neither of them changing any source code

Figured out that was due to an accidental ESLint upgrade in #534's yarn.lock (#534 (comment)) which I then reverted and got that one passing and merged.

So this is no longer needed but may be if ESLint is upgraded. Kicking this back to draft due to the above issue then.

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

Successfully merging this pull request may close these issues.

2 participants