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.

Sign up for GitHub

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

Jump to bottom

Fix stacked plot child selection #6275

Merged
merged 12 commits into from
Feb 3, 2023
Merged

Fix stacked plot child selection #6275

merged 12 commits into from
Feb 3, 2023

Conversation

shefalijoshi
Copy link
Contributor

@shefalijoshi shefalijoshi commented Feb 3, 2023 •
edited by ozyx
Loading

Closes #6261

Describe your changes:

When there are no annotations and no new annotation selections, make sure the plot is still selected.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@scottbell scottbell mentioned this pull request Feb 3, 2023
Closed
scottbell
scottbell approved these changes Feb 3, 2023
View reviewed changes
Copy link
Contributor

@scottbell scottbell left a comment

Choose a reason for hiding this comment

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

LGTM

@scottbell
Copy link
Contributor

Screen.Recording.2023-02-03.at.6.51.23.PM.mov

@codecov
Copy link

codecov bot commented Feb 3, 2023 •
edited
Loading

Codecov Report

Merging #6275 (4ca6d45) into master (0f312a8) will decrease coverage by 0.01%.
The diff coverage is 43.33%.

❗ Current head 4ca6d45 differs from pull request most recent head 5d2c2e0. Consider uploading reports for the commit 5d2c2e0 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6275      +/-   ##
==========================================
- Coverage   55.17%   55.17%   -0.01%     
==========================================
  Files         607      607              
  Lines       26088    26090       +2     
  Branches     2376     2376              
==========================================
  Hits        14394    14394              
- Misses      11026    11027       +1     
- Partials      668      669       +1
Flag Coverage Δ *Carryforward flag
e2e-ci 39.49% <ø> (ø) Carriedforward from b875581
e2e-full 51.23% <ø> (ø) Carriedforward from b875581
e2e-stable 54.18% <ø> (+0.25%) ⬆️
unit 49.73% <43.33%> (+<0.01%) ⬆️ Carriedforward from b875581

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
src/plugins/plot/MctPlot.vue 37.66% <0.00%> (-0.12%) ⬇️
src/plugins/plot/inspector/PlotOptionsBrowse.vue 75.43% <ø> (ø)
src/plugins/plot/inspector/PlotOptionsEdit.vue 58.00% <ø> (ø)
src/plugins/plot/inspector/forms/YAxisForm.vue 52.00% <ø> (ø)
src/ui/inspector/InspectorViews.vue 64.28% <ø> (ø)
src/plugins/plot/stackedPlot/StackedPlotItem.vue 60.60% <61.90%> (ø)
src/plugins/plot/configuration/SeriesCollection.js 63.36% <0.00%> (-11.89%) ⬇️
.../plugins/timeConductor/ConductorInputsRealtime.vue 50.00% <0.00%> (-1.20%) ⬇️
...ettingsSynchronizer/URLTimeSettingsSynchronizer.js 96.96% <0.00%> (-1.02%) ⬇️
src/plugins/timeConductor/ConductorHistory.vue 51.48% <0.00%> (-1.00%) ⬇️
... and 9 more

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 0f312a8...5d2c2e0. Read the comment docs.

ozyx
ozyx suggested changes Feb 3, 2023
View reviewed changes
Copy link
Contributor

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left one refactor suggestion, and we should add some e2e coverage for this before we merge.

@shefalijoshi shefalijoshi requested a review from ozyx February 3, 2023 18:42
ozyx
ozyx suggested changes Feb 3, 2023
View reviewed changes
Copy link
Contributor

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Looking good so far. I noticed that we can only switch between properties of objects if clicking on them while not in edit mode. Once in edit mode, if you click the canvas, it will switch to the parent object. SO we are unable to switch between elements while in edit mode, something I think we were able to do previously?

Other than that, got some suggestions for improving the e2e test and selectors.

@shefalijoshi shefalijoshi requested a review from ozyx February 3, 2023 22:24
ozyx added 4 commits February 3, 2023 14:36
@ozyx
refactor: remove unnecessary awaits
78323aa
@ozyx
a11y: move aria-label to StackedPlotItem
faf42e1
@ozyx
refactor(e2e): combine like tests, unique object names
89277d4 
- Use unique object names in `text=` selectors

- Combine like tests to reduce execution time

- Use `getByRole` selectors where able
@ozyx
docs(e2e): add comments to test
4ca6d45
@ozyx ozyx enabled auto-merge (squash) February 3, 2023 23:31
@ozyx ozyx disabled auto-merge February 3, 2023 23:45
@ozyx ozyx enabled auto-merge (squash) February 3, 2023 23:51
@ozyx ozyx merged commit be38c3e into master Feb 3, 2023
@ozyx ozyx deleted the fix-stacked-plot-child-selection branch February 3, 2023 23:56
shefalijoshi added a commit that referenced this pull request Feb 5, 2023
@shefalijoshi @scottbell @ozyx
cherry-pick(#6275): Fix stacked plot child selection (#6275)
ba485c9 
* Fix selections for different scenarios

* Ensure plot selection in stacked plots works when there are no selected or found annotations

* Adds e2e test for stacked plot selection and fixes the old e2e test which was testing overlay plots instead.

* Fix selection of plots while in Edit mode

* Improve tests for stacked plots

* refactor: remove unnecessary `await`s

* a11y: move aria-label to StackedPlotItem

* refactor(e2e): combine like tests, unique object names

- Use unique object names in `text=` selectors

- Combine like tests to reduce execution time

- Use `getByRole` selectors where able

* docs(e2e): add comments to test

* fix: add class back for unit test selector

---------

Co-authored-by: Scott Bell <[email protected]>
Co-authored-by: Jesse Mazzella <[email protected]>
ozyx pushed a commit that referenced this pull request Feb 5, 2023
@shefalijoshi
cherry-pick(#6275): Fix stacked plot child selection (#6283)
2d502b7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozyx ozyx ozyx approved these changes

@scottbell scottbell scottbell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stacked Plots don't allow child selection while editing
3 participants
@shefalijoshi @scottbell @ozyx