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

add data-testid to connection form fields #3495

Merged

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Nov 19, 2024

Description

Adds field <env var> data-testid to field control elements.
Replaces generated field IDs with field-<env var> values to ensure predicable IDs can be used with bitwarden.
Add data-testid to sections.

image

How Has This Been Tested?

Web inspector

Test Impact

Updated unit tests.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@emilys314
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 19, 2024
@openshift-ci openshift-ci bot removed the lgtm label Nov 20, 2024
@christianvogt christianvogt force-pushed the ct-fields-data-testid branch 2 times, most recently from 4302a05 to 11f7be8 Compare November 20, 2024 15:04
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 61.11111% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.23%. Comparing base (3ba248d) to head (73ab58b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../concepts/connectionTypes/fields/TextFormField.tsx 25.00% 6 Missing ⚠️
...epts/connectionTypes/fields/DataFormFieldGroup.tsx 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3495      +/-   ##
==========================================
- Coverage   85.24%   85.23%   -0.02%     
==========================================
  Files        1355     1355              
  Lines       31074    31078       +4     
  Branches     8663     8663              
==========================================
- Hits        26489    26488       -1     
- Misses       4585     4590       +5     
Files with missing lines Coverage Δ
...onnectionTypes/fields/ConnectionTypeFormFields.tsx 100.00% <100.00%> (ø)
...ncepts/connectionTypes/fields/SectionFormField.tsx 50.00% <100.00%> (ø)
...epts/connectionTypes/fields/DataFormFieldGroup.tsx 80.00% <80.00%> (-5.72%) ⬇️
.../concepts/connectionTypes/fields/TextFormField.tsx 50.00% <25.00%> (-50.00%) ⬇️

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa74ed9...73ab58b. Read the comment docs.

---- 🚨 Try these New Features:

@christianvogt
Copy link
Contributor Author

@emilys314 there was a conflict with test ids and one of the cypress tests so I made a new change to use a static test id for the control element which can be chained from the parent selector. Also noticed that one element did not pass through the test id.

@emilys314
Copy link
Contributor

yeah that makes sense.

I don't know if this version would fix Fede's and Jorge's original issue of the input fields themselves not having a unique identifier in the dom though. I don't know if bitwarden is able to do chaining.

Also the original data connections had the data-testid on the input themselves and not on the parent form group, is there any reason we have them there instead? (I suppose you can narrow down different elements for more complicated fields)

Screenshot From 2024-11-20 11-02-10

@christianvogt
Copy link
Contributor Author

data test IDs are not meant to be unique, we often use them as unique identifiers however the same selection can be achieve through narrowing. For the sake of simplicity I will add the env var to the testid as well.

I hadn't planned on tackling the bitwarden issue with unique ids here, but I suppose I can do so.

@christianvogt christianvogt force-pushed the ct-fields-data-testid branch 2 times, most recently from a3edc72 to a06d0a4 Compare November 20, 2024 21:02
@emilys314
Copy link
Contributor

/lgtm

image

Copy link
Contributor

openshift-ci bot commented Nov 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 2d9a5f5 into opendatahub-io:main Nov 21, 2024
6 checks passed
@jgarciao
Copy link

I've tested today's odh-nightly and now I'm able again to create bitwarden custom fields to populate the values. Thanks a lot, this will save us plenty of time!

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

Successfully merging this pull request may close these issues.

3 participants