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

857/chart style #903

Merged
merged 7 commits into from
Apr 2, 2024
Merged

857/chart style #903

merged 7 commits into from
Apr 2, 2024

Conversation

hanbyul-here
Copy link
Collaborator

@hanbyul-here hanbyul-here commented Mar 24, 2024

Related Ticket: #857

Description of Changes

Change new E&A Chart style

Notes & Questions About Changes

Part of #857
The list of changes specified in Figma: https://www.figma.com/file/9INQauBWhiRxvOWDGhRrxO/US-GHG-Center?type=design&node-id=1127-2201&mode=design&t=VNfErIVXtEgHmbZi-0

  • On the chart, average and standard deviation are shown by default.
  • Standard deviation is shown as a grey band around the average line.
  • Median, min and max are optional, accessed through the “Chart options” icon. (*Unless editor specified which analysis to show)
  • Min and max are shown as grey dotted lines.
  • The y-axis is shown on the gray area outside the analysis period.
  • The y-axis should always show the zero line
  • Always expand with yaxis. Get rid of 'collapse' button
  • Optional: The chart actions icons are:”Download chart”, “Chart options” and “Collapse”. Collapse is a local action and collapses the current chart only. - only implemented "Chart Options" for now

Question: What should we do for circles that mark the time unit? If I remember correctly, we added it because of the requests from stakeholders.

Validation / Testing

To check the changes, run analysis on this page

Copy link

netlify bot commented Mar 24, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 0e3a458
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6605ed657293ec00087e638d
😎 Deploy Preview https://deploy-preview-903--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hanbyul-here hanbyul-here marked this pull request as ready for review March 26, 2024 06:12
Copy link

@faustoperez faustoperez left a comment

Choose a reason for hiding this comment

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

Great work Hanbyul!

One thing about the tooltip behavior: if someone hovers on the chart there should always be an "active node" so that the tooltip is present, no matter the mouse position. At the moment, it disappears if the mouse position is not near a node. I think that's the reason the little dots on the line charts were included in the first place.

Frame 2516

Proposed fix: the tooltip should always be present when hovering over the chart

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Mar 26, 2024

Did a quick lookover, code lgtm 👍🏼 I can re-review again though once updated suggestions are made!

@hanbyul-here
Copy link
Collaborator Author

@faustoperez I handled the hovering issue 💦

@faustoperez
Copy link

@hanbyul-here Great job, I wasn't able to replicate the missing tooltip behavior ✊

One thing that's missing is the vertical line on the active node so users have a reference of the hover point, as in the designs. Also, the dots for the St Deviation are missing.

Implementation:
image

Designs:
image

Thank you! 💐

@hanbyul-here
Copy link
Collaborator Author

@sandrahoang686 This PR is ready for reviews 🙇

@j08lue
Copy link
Contributor

j08lue commented Mar 28, 2024

Looks amazing! 😍 This is a huge improvement to the scientific value of these charts.

Please note - labels on the chart value tooltip should be spelled out, please:

  1. "Average"
  2. "St Deviation"
image image

Copy link
Collaborator

@sandrahoang686 sandrahoang686 left a comment

Choose a reason for hiding this comment

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

teeny comments otherwise lgtm :shipit:

Copy link

@faustoperez faustoperez 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 Hanbyul great work 🙌

@hanbyul-here hanbyul-here merged commit 694a665 into main Apr 2, 2024
8 checks passed
@hanbyul-here hanbyul-here deleted the 857/chart-style branch April 2, 2024 11:07
@j08lue
Copy link
Contributor

j08lue commented Apr 2, 2024

This also closed

hanbyul-here added a commit that referenced this pull request May 1, 2024
## 🎉 Features
- Zoom in AOI, TOI when analysis is run in
#906
- Add custom javascript injection
#846
- ADR for V2 Refactor: #875
- Open all external links in a new tab in
#870
- Include dataset id to filter layers in
#910
- Some datasets can only be analyzed with layers from the same source in
#913
- Create minimal partial data layer scaffold starting off with Data
Catalog for VEDA2 Refactor in
#893
- Add analysis preset in #921

## 🚀 Improvements
- Chart style improvement in
#903
- Data Catalog enhancement with floating filter sidebar in
#918
- Sum as statistics option in
#925
-
## 🐛 Fixes
- Sort featured stories based on publication date in descending order in
#907
- Replace latency with temporalResolution in layer info in
#898
- Add a workaround for Safari scroll problem in
#909
- Handle empty result in #922
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.

4 participants