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

Adjusting rescale value for small/large numbers #1272

Merged
merged 9 commits into from
Dec 4, 2024
Merged

Conversation

snmln
Copy link
Contributor

@snmln snmln commented Nov 22, 2024

Related Ticket: https://github.com/orgs/NASA-IMPACT/projects/17?pane=issue&itemId=85258058&issue=NASA-IMPACT%7Cveda-ui%7C1226

Description of Changes

  • Adjusting the text input to be wider.
  • Cleaning out colorRangeSlider component, creating util file, creating initial tests.
  • Creating dynamic step calculation functionality

Notes & Questions About Changes

When observing a value in either the max or min inputs if that value has a character count of 6 or greater the component will convert that number into scientific notation as a display value.

Validation / Testing

Run this branch on the veda-config code base, navigate to the E&A page and select the difference CO2 dataset. This is an extreme data set that should display all changes in this component.

  • Confirm that the middle range identifier is always at halfway mark.
  • Confirm that the numbers are displaying in scientific notation.

Copy link

netlify bot commented Nov 22, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit dad42ea
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/674dc91fcf54640008e268ba
😎 Deploy Preview https://deploy-preview-1272--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.

@aboydnw
Copy link
Contributor

aboydnw commented Nov 22, 2024

@snmln I assume you're looking at NO2 diff, not CO2 diff. Just commenting here for posterity, I think this is an acceptable visualization of these small/large numbers.

image

Only question is, were you able to test this out where the magnitude of the min/max were both <0.01?

@snmln
Copy link
Contributor Author

snmln commented Nov 22, 2024

CO2 will also display a 1.5e-6 value I believe, so both NO2 and CO2 will work for testing purposes. And yes, I was able to confirm the 2 extremes of the data visualization will present as desired.
Screenshot 2024-11-22 at 2 43 42 PM

Copy link
Contributor

@aboydnw aboydnw left a comment

Choose a reason for hiding this comment

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

Manual testing looks good on this one. Still need a review from an engineer

@snmln snmln linked an issue Nov 27, 2024 that may be closed by this pull request
2 tasks
@snmln snmln force-pushed the fix/color-range-slider branch from 85f994b to 7fdb52e Compare November 27, 2024 18:50
@snmln snmln force-pushed the fix/color-range-slider branch from 459eef5 to 1d4d878 Compare November 27, 2024 19:16
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This work was previously approved in: #1190. Then reverted when bug was identified. I needed to pull those changes back in to correctly reflect updates.

@vgeorge
Copy link
Contributor

vgeorge commented Nov 28, 2024

@snmln when visiting the dataset you recommended, I was able to see that the inputs are always on scientific notation and I was able to edit then. However, I noticed a couple of small things:

  • When entering values outside the range, the error message displays them on a different format:
Screenshot 2024-11-28 at 10 00 02
  • Rescale changes are no applied after changing the slides, I needed to switch color scales to see the changes reflected on the map:

rescale

@snmln
Copy link
Contributor Author

snmln commented Dec 2, 2024

@vgeorge Thank you for these call out! I wanted to respond to a few:

When entering values outside the range, the error message displays them on a different format:
This is purposeful.

The scientific notation is a solution to ensure as much of the number is visible within the boundaries of the input space as possible. We still want to display the numbers in non-scientific notation outside of the input box. This will also only happen when the number in place is 7 characters or larger.

Rescale changes are no applied after changing the slides, I needed to switch color scales to see the changes reflected on the map:

Good catch there was a few raster-layer changes that got lost in the shuffle with re-implementing the changes from #1190. I have gone ahead an implemented them and it should now be functioning correctly.

@snmln snmln merged commit a35a28e into main Dec 4, 2024
9 checks passed
@snmln snmln deleted the fix/color-range-slider branch December 4, 2024 20:36
@snmln snmln mentioned this pull request Dec 6, 2024
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.

Adjust rescale values for small/large numbers
3 participants