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

change notification for truncated axis #397

Merged
merged 4 commits into from Sep 9, 2021
Merged

Conversation

moontrip
Copy link
Contributor

@moontrip moontrip commented Sep 7, 2021

This issue replies on the corresponding PR at web-components. Details can be found at the PR.

);
}
}, [truncationConfigIndependentAxisMin, truncationConfigIndependentAxisMax]);

useEffect(() => {
if (truncationConfigDependentAxisMin || truncationConfigDependentAxisMax) {
setTruncatedDependentAxisWarning(
'Data may have been truncated (light gray area) by range selection'
' Data may have been truncated (light gray area) by range selection'
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice not to need the leading space. Can the Notification component take care of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobular thank you for your review and comments 👍 I will try to make this at Notification component level.

@@ -548,15 +548,15 @@ function HistogramPlotWithControls({
truncationConfigIndependentAxisMax
) {
setTruncatedIndependentAxisWarning(
'Data has been truncated (light gray area) by range selection'
' Data has been truncated (light gray area) by range selection'
Copy link
Member

@bobular bobular Sep 8, 2021

Choose a reason for hiding this comment

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

I don't really like how these warnings read. I'd prefer something like

Data has been truncated by range selection, as indicated in the plot by the light gray shading.

What do @danicahelb and @SheenaTomko think?

I know that's very long, but there is space for two lines of text in the blue notification area. I tried a longer message and it will need some CSS changes so that it wraps more aggressively and doesn't make the blue box much wider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobular will try. It would be a bit tricky to change CSS though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobular As suggested I have made single space at Notification component so the single space at histogram filter is removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bobular also adjusted notification size with the new message by setting style from histogram filter component. Like,

histogramfilter-update1

@SheenaTomko
Copy link
Contributor

I might change it slightly to "Data has been truncated by range selection, as indicated by the light gray shading."

Copy link
Member

@bobular bobular left a comment

Choose a reason for hiding this comment

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

That all looks great now, thanks!

@moontrip moontrip merged commit f4fb700 into main Sep 9, 2021
@dmfalke dmfalke deleted the truncated-axis-update branch March 6, 2023 13:03
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.

Pop-up box when truncating axes on histograms in the subsetting tab
4 participants