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(graphCardMetricTotals): ent-4366 expand data-test #838

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

cdcabrera
Copy link
Member

What's included

  • fix(graphCardMetricTotals): ent-4366 expand data-test

Notes

  • applies a data-test="graphMetricTotals" attribute to the div surrounding daily, monthly, and the graph display for individual metric displays, currently used in RHOSAK. @mirekdlugosz @ntkathole

How to test

Proxy run check

  1. update the NPM packages with $ yarn
  2. make sure Docker is running, plus on network, then
  3. $ yarn start:proxy
  4. confirm the data-test attribute appears in the surrounding div

Example

...

Updates issue/story

ent-4366

@cdcabrera cdcabrera added the 202112 project phase label Nov 18, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #838 (5dedce2) into ci (b73b0c2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##               ci     #838   +/-   ##
=======================================
  Coverage   94.02%   94.02%           
=======================================
  Files         123      123           
  Lines        3514     3514           
  Branches     1344     1344           
=======================================
  Hits         3304     3304           
  Misses        193      193           
  Partials       17       17           
Impacted Files Coverage Δ
src/components/graphCard/graphCardMetricTotals.js 94.28% <ø> (ø)

Continue to review full report at Codecov.

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

Copy link
Collaborator

@mirekdlugosz mirekdlugosz left a comment

Choose a reason for hiding this comment

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

Would it be possible to have different data-test for each group? So (example)

  • Data transfer gets graphMetricTotalsDataTransfer
  • Data storage gets graphMetricTotalsDataStorage
  • Instance hours gets graphMetricTotalsInstanceHours

Current patch is still helpful and worthwhile, as it will allow me to target groups. But then I will have to rely on their order, which is sub-optimal. Having unique attributes would solve all my problems.

I think unique attributes require passing a prop from parent and handling it, and I understand it would be more work. So I'm fine with merging current version, which is still an improvement, and getting back to this once timelines are not so tight.

@cdcabrera
Copy link
Member Author

@mirekdlugosz we can pass in the "metric identifier" and we can try to run lodash's "camel case" method on them to generate a unique data-test attribute... but the limitation is they would be dependent on whatever naming the API provides... I'll add the code in so you can get an idea, we can always not push it

@mirekdlugosz
Copy link
Collaborator

Depending on API naming is not ideal, as we strive for having stable identifier here. On the other hand, API is unlikely to change (very often), so it's probably better than nothing.

We don't have to run it through lodash, unless you want it for consistency. As far as QE is concerned, this can be randomly-generated string. As long as it uniquely identifies element on page and is stable across deployments and environments, we are good. graphMetricTotalsDataTransfer, graphMetricTotals-data-transfer and e19dc88d6235 are all equally good for us.

@cdcabrera cdcabrera force-pushed the ent-4366-datatestattrs branch from 027f6a8 to 5dedce2 Compare November 29, 2021 16:35
@cdcabrera
Copy link
Member Author

cdcabrera commented Nov 29, 2021

@mirekdlugosz

The final format came down to looping through each products requested display metric ids. Since this can vary between products and we can't always guarantee the format for the Metric ID the API is going to use the data-test attribute follows as

  • attribute, data-test={graphMetricTotals-${_camelCase(metricId)}} ... example, say metric ID equals Transfer-gibibytes and results in, data-test="graphMetricTotals-transferGibibytes"

Where we run lodash camelCase for a minor bit of consistency in case a space is used instead of a dash, and apply a graphMetricTotals- to help with a partial attribute search using CSS selectors

  • document.querySelector('[data-test^=graphMetricTotals]')

We can always update the attribute if say the API always guarantees they'll use a dash instead of potentially a blank/space..

@cdcabrera cdcabrera merged commit 45c2914 into RedHatInsights:ci Nov 29, 2021
@mirekdlugosz
Copy link
Collaborator

@cdcabrera I see changes already on CI. That's exactly what I meant! It's perfect, thank you.

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

Successfully merging this pull request may close these issues.

3 participants