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

argoproj / argo-ui

Argoproj shared React components

Apache License 2.0

220 stars 178 forks source link

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

Open agilgur5 opened 6 months ago

agilgur5 commented 6 months ago

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

agilgur5 commented 6 months ago

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 commented 6 months ago

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?

github-actions[bot] commented 4 months ago

Stale pull request message

agilgur5 commented 3 months ago

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 (https://github.com/argoproj/argo-ui/pull/534#issuecomment-2106058288) 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.