Plots correctly use configuration set on the parent if they can't their own #7770
Conversation
… is correct initialized by the parent.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7770 +/- ##
==========================================
- Coverage 56.95% 56.70% -0.26%
==========================================
Files 673 673
Lines 27177 27225 +48
Branches 2636 2663 +27
==========================================
- Hits 15479 15437 -42
- Misses 11367 11447 +80
- Partials 331 341 +10
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…ry points are applied correctly
| .getByRole('list', { name: 'Plot Series Properties' }) | ||
| .locator('[title="Alarm Markers"]>div') | ||
| .nth(1) | ||
| ).toContainText('Disabled'); |
There was a problem hiding this comment.
We should find a better way of locating this element without using the locator method, nth, or title locators.
Ideally, this assertion should look like this:
await expect(await page.getByRole('listitem', { name: object.name }).getByLabel('Alarm Marker's).toContainText('Disabled')
| @@ -27,6 +27,14 @@ async function turnOffAutoscale(page) { | |||
| await page.getByRole('checkbox', { name: 'Auto scale' }).uncheck(); | |||
| } | |||
|
|
|||
| async function turnOffAlarmMarkers(page) { | |||
There was a problem hiding this comment.
we should remove this.
Tests should be more readable with
await page.getByLabel('Alarm Markers').uncheck();
| @@ -39,19 +39,23 @@ test.describe('Stacked Plot', () => { | |||
| await page.goto('./', { waitUntil: 'domcontentloaded' }); | |||
|
|
|||
| stackedPlot = await createDomainObjectWithDefaults(page, { | |||
| type: 'Stacked Plot' | |||
| type: 'Stacked Plot', | |||
| name: 'Stacked Plot' | |||
There was a problem hiding this comment.
added names to be explicit in the selection of objects
| await page.getByRole('tab', { name: 'Config' }).click(); | ||
|
|
||
| // Click on canvas for the 1st plot | ||
| await page.getByLabel(`Stacked Plot Item ${swgA.name}`).click(); |
There was a problem hiding this comment.
updated this to use the getByLabel method, directly
| await page.getByLabel('Expand Sine Wave Generator').click(); | ||
|
|
||
| // turn off alarm markers. Technically there should be a getBy locator to ensure we're selecting the correct alarm marker | ||
| await page.getByLabel('Alarm Markers').uncheck(); |
There was a problem hiding this comment.
Technically this should be a chained getBy locator to ensure we're selecting the correct alarm marker
| @@ -21,7 +21,7 @@ | |||
| --> | |||
| <template> | |||
| <div v-if="loaded" class="js-plot-options-browse"> | |||
| <ul v-if="!isStackedPlotObject" class="c-tree" aria-label="Plot Series Properties"> | |||
There was a problem hiding this comment.
aria labels implies interactivity. This is not an interactive component
| > | ||
| <h2 class="--first" title="Legend options">Legend</h2> | ||
| <legend-form class="grid-properties" :legend="config.legend" /> | ||
| <legend-form role="treeitem" tabindex="0" class="grid-properties" :legend="config.legend" /> |
There was a problem hiding this comment.
allow users to tab to this component
| <span | ||
| class="c-disclosure-triangle is-enabled flex-elem" | ||
| :class="expandedCssClass" | ||
| role="button" | ||
| :aria-label="ariaLabelValue" | ||
| tabindex="0" |
| @@ -28,8 +28,10 @@ | |||
| role="tab" | |||
| class="c-inspector__tab c-tab" | |||
| :class="{ 'is-current': isSelected(tab) }" | |||
| tabindex="0" | |||
There was a problem hiding this comment.
this allows us to tab to the plot series expand/collapse component
| page | ||
| .getByTitle('Display markers visually denoting points in alarm.') | ||
| .getByRole('cell', { name: 'Disabled' }) | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
This is a chained locator which leverages Titles
| @@ -84,10 +88,15 @@ | |||
| </li> | |||
| <li class="grid-row"> | |||
| <div class="grid-cell label" title="Display markers visually denoting points in alarm."> | |||
| Alarm Markers | |||
| <label for="alarm-markers-checkbox">Alarm Markers</label> | |||
There was a problem hiding this comment.
@ozyx here's an attempt at using labels and ids
There was a problem hiding this comment.
what happens in the case of an overlay plot with multiple series to configure?
|
@shefalijoshi Looks good to me, thank you. Glad it was a relatively small code change. |
| @@ -21,7 +21,7 @@ | |||
| --> | |||
| <template> | |||
| <ul> | |||
| <li class="c-tree__item menus-to-left" :class="aliasCss"> | |||
| <li class="c-tree__item menus-to-left" :class="aliasCss" role="treeitem"> | |||
There was a problem hiding this comment.
this probably will break some tests (for good reason), i think the only other thing that has treeitem role is the main tree. but this seems like the right role for this to have
| @@ -21,7 +21,7 @@ | |||
| --> | |||
| <template> | |||
| <div v-if="loaded" class="js-plot-options-browse"> | |||
| <ul v-if="!isStackedPlotObject" class="c-tree" aria-label="Plot Series Properties"> | |||
| <ul v-if="!isStackedPlotObject" class="c-tree" role="tree"> | |||
| @@ -122,8 +122,11 @@ export default { | |||
| 'grid-lines', | |||
| 'plot-y-tick-width' | |||
| ], | |||
| mounted() { | |||
| beforeMount() { | |||
| // We must do this before mounted to use any series configuration options set at the stacked plot level | |||
There was a problem hiding this comment.
y tho? is it because something happens to the configStore when the parent mounts?
…efactor out `plotActions.js`
Closes #7771
Describe your changes:
For telemetry that cannot have it's own configuration, ensure that it is correct initialized by the parent.
All Submissions:
Author Checklist
type:label? Note: this is not necessarily the same as the original issue.Reviewer Checklist