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

Toggle between showing aggregate stacked plot legend or per-plot legend #6758

Merged
merged 8 commits into from
Jul 11, 2023

Conversation

shefalijoshi
Copy link
Contributor

@shefalijoshi shefalijoshi commented Jun 23, 2023

Closes #6792 and VIPERRQSTS-239

Describe your changes:

New option to show/hide stacked plot aggregate legend - defaulted to …not show.
Use the Plot component in the StackedPlotItem component for simplicity
Add logic to how/hide per plot legends as needed.

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)

…not show.

Use the Plot component in the StackedPlotItem component for simplicity and show/hide sub-legends as needed.
@deploysentinel
Copy link

deploysentinel bot commented Jun 23, 2023

Current Playwright Test Results Summary

✅ 13 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 07/11/2023 11:16:33pm UTC)

Run Details

Running Workflow e2e-couchdb on Github Actions

Commit: f64d566

Started: 07/11/2023 11:13:40pm UTC

⚠️ Flakes

📄   functional/plugins/notebook/notebookWithCouchDB.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Notebook Tests with CouchDB @couchdb Inspect Notebook Entry Network Requests
Retry 1Initial Attempt
15.38% (4) 4 / 26 runs
failed over last 7 days
65.38% (17) 17 / 26 runs
flaked over last 7 days

View Detailed Build Results


Current Playwright Test Results Summary

✅ 123 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 07/11/2023 11:16:33pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: f64d566

Started: 07/11/2023 10:25:48pm UTC

⚠️ Flakes

📄   functional/plugins/imagery/exampleImagery.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Example Imagery Object Can use Mouse Wheel to zoom in and out of latest image
Retry 1Initial Attempt
6.41% (5) 5 / 78 runs
failed over last 7 days
74.36% (58) 58 / 78 runs
flaked over last 7 days

View Detailed Build Results


@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #6758 (f64d566) into master (293f25d) will increase coverage by 0.00%.
The diff coverage is 65.11%.

@@           Coverage Diff           @@
##           master    #6758   +/-   ##
=======================================
  Coverage   53.75%   53.76%           
=======================================
  Files         624      624           
  Lines       24848    24879   +31     
  Branches     2493     2498    +5     
=======================================
+ Hits        13358    13375   +17     
- Misses      10822    10833   +11     
- Partials      668      671    +3     
Flag Coverage Δ *Carryforward flag
e2e-full 42.17% <ø> (-0.02%) ⬇️ Carriedforward from 13681ba
e2e-stable 55.58% <ø> (+0.33%) ⬆️
unit 48.49% <65.11%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
src/plugins/plot/configuration/LegendModel.js 90.00% <ø> (ø)
src/plugins/plot/inspector/PlotOptionsEdit.vue 51.56% <ø> (ø)
...rc/plugins/plot/legend/PlotLegendItemCollapsed.vue 61.36% <0.00%> (ø)
src/plugins/plot/stackedPlot/StackedPlotItem.vue 35.20% <33.33%> (-2.90%) ⬇️
src/plugins/plot/stackedPlot/StackedPlot.vue 55.00% <50.00%> (-5.68%) ⬇️
src/plugins/plot/Plot.vue 50.00% <76.47%> (+8.46%) ⬆️
src/plugins/plot/MctPlot.vue 36.77% <100.00%> (-0.14%) ⬇️
src/plugins/plot/chart/MctChart.vue 46.50% <100.00%> (+0.24%) ⬆️
src/plugins/plot/inspector/PlotOptionsBrowse.vue 83.33% <100.00%> (+0.28%) ⬆️
src/plugins/plot/inspector/forms/LegendForm.vue 51.61% <100.00%> (+5.18%) ⬆️

... and 8 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 293f25d...f64d566. Read the comment docs.

Copy link
Contributor

@rukmini-bose rukmini-bose left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@akhenry akhenry requested a review from jvigliotta June 28, 2023 17:43
Copy link
Contributor

@jvigliotta jvigliotta left a comment

Choose a reason for hiding this comment

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

Two small changes and 2 issues I found. It seams staleness styling is a little messed up now (example images) and the other issue is that position of the legend doesn't seem to be working anymore?

Before:
Screenshot 2023-07-03 at 3 47 57 PM

After:
Screenshot 2023-07-03 at 3 46 59 PM

This is the same plot, but you can see the position is set to bottom which is working in the older version. The issue with styling seems to be the border is missing and the legend item styling seems off.

src/plugins/plot/chart/MctChart.vue Outdated Show resolved Hide resolved
src/plugins/plot/Plot.vue Outdated Show resolved Hide resolved
@rukmini-bose
Copy link
Contributor

ues I found. It seams staleness styling is a little messed up now (example images) and the other issue is that position of the legend doesn't seem to be working anymore?

I took a look at the staleness issue. It seems that <div class="l-view-section u-style-receiver js-style-receiver">, the outer shell of the stale plot, is not receiving the is-stale class anymore. Is this a bug?

And Jamie's observation about the legend positions not working is true– they don't seem to work anymore.

@shefalijoshi shefalijoshi requested a review from jvigliotta July 7, 2023 17:46
@rukmini-bose rukmini-bose self-requested a review July 11, 2023 17:29
Copy link
Contributor

@rukmini-bose rukmini-bose left a comment

Choose a reason for hiding this comment

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

It looks like the staleness bux is half fixed. While we now see the correct border an icon at the bottom left of the plot, we also see the icon on the legend, which shouldn't be there. I believe that in the legend, the data point should only be highlighted in cyan.

What we are seeing (notice the legend):
Screen Shot 2023-07-11 at 10 38 38 AM

What it should look like:
Screen Shot 2023-07-11 at 10 40 22 AM

In addition, I had a thought– if users cannot change the positioning of the legend when we are the "per plot" setting is toggled on (which was a conscious UI decision), can we just hide that setting unless the user toggles off the setting? Otherwise, I can see how it can cause confusion to a user.

Other than that, all looks good!

@jvigliotta
Copy link
Contributor

I didn't see anything other than what @rukmini-bose pointed out. I did notice, you only get the messed up icons when "per plot" legend is selected. It's fine when it's all in one line.

Copy link
Contributor

@rukmini-bose rukmini-bose left a comment

Choose a reason for hiding this comment

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

This looks good! For a future enhancement, let’s try to disable the option for users to change the position of the legend if it’s per plot. LGTM, good job :))

Copy link
Contributor

@jvigliotta jvigliotta 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!

@jvigliotta jvigliotta enabled auto-merge (squash) July 11, 2023 20:48
@jvigliotta jvigliotta added the pr:e2e:couchdb npm run test:e2e:couchdb label Jul 11, 2023
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jul 11, 2023
@jvigliotta jvigliotta merged commit d08ea62 into master Jul 11, 2023
@jvigliotta jvigliotta deleted the stackedplot-legend-display-options branch July 11, 2023 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggle between showing aggregate stacked plot legend or per-plot legend
3 participants