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

UI/d3 DOM cleanup hover issue #14493

Merged
merged 10 commits into from
Mar 16, 2022
Merged

UI/d3 DOM cleanup hover issue #14493

merged 10 commits into from
Mar 16, 2022

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Mar 15, 2022

This hover bug was happening because not all of the D3 elements were removed from the page when the chart updated
chart-hover-bug

cleaned up HTML into groups with fix
cleanup-chart-html

@vercel vercel bot temporarily deployed to Preview – vault March 15, 2022 04:37 Inactive
@hellobontempo hellobontempo changed the title UI/d3 element bug UI/d3 DOM cleanup hover issue Mar 15, 2022
@@ -292,7 +337,7 @@ module('Acceptance | clients history tab', function (hooks) {
assert.equal(currentURL(), '/vault/clients/history', 'clients/history URL is correct');
assert
.dom(SELECTORS.emptyStateTitle)
.includesText('No start date found', 'Empty state shows no billing start date');
.includesText('start date found', 'Empty state shows no billing start date');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for enterprise this reads "No billing start date found" and for OSS "No start date found"

this was previously passing, but started failing for some reason? so only checking for the words shared between both versions

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you running Vault enterprise locally with the tests (assuming it's failing locally?). The tests run on whatever version of vault you have loaded so that could be why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - I am. But I thought I had changed this from hasText to includesText in a previous PR so that as long as "No...start date found" was present the test would pass.

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for finding and fixing!

// Filter by namespace

// check chart displays correct elements and values
for (const key in CHART_ELEMENTS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@vercel vercel bot temporarily deployed to Preview – vault March 15, 2022 15:29 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 15, 2022 15:34 Inactive
@hellobontempo hellobontempo added the bug Used to indicate a potential bug label Mar 16, 2022
@hashishaw hashishaw merged commit bc99b9f into main Mar 16, 2022
@hashishaw hashishaw deleted the ui/client-count-1.10-demo branch March 16, 2022 18:37
hellobontempo added a commit that referenced this pull request Mar 16, 2022
* fix duplicate rendering of chart elements

* organize SVG char elements into groups, give data-test attrs

* update tests

* tweak mirage

* add fake client counting start date

* fix test

* add waitUntil

* adds changelog

* add second waituntil
hellobontempo added a commit that referenced this pull request Mar 16, 2022
* fix duplicate rendering of chart elements

* organize SVG char elements into groups, give data-test attrs

* update tests

* tweak mirage

* add fake client counting start date

* fix test

* add waitUntil

* adds changelog

* add second waituntil
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport bug Used to indicate a potential bug ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants