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

Exploration improvements from feedback. #767

Merged
merged 18 commits into from
Dec 4, 2023

Conversation

danielfdsilva
Copy link
Collaborator

Items discussed and listed in the figma file. cc @faustoperez @j08lue

  • Rename datasets to data layers ✅
  • Add layer button label ✅
  • Compare dates button ✅
  • Add label to map analysis tools / Remove disabled trash and refresh icons from analysis icon group ⚠️
  • Add tooltips to icons ✅
  • Add zoom controls to map ⚠️
    • Zoom controls added to the map, but not possible to add tooltips because it is a mapbox component. It would be necessary to create a custom component.
  • Add zoom controls to timeline ✅
  • Add horizontal scroll to timeline if needed ❌
    • Given the complexity of this (need a custom scrollbar) will postpone until further testing
  • Add analysis banners -- add primary button to start analysis ❌
  • Update card styles on data catalog, change button label from “Confirm” to “Add to map” ✅

Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit c3b3044
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6568a242d581e50008e28c8d
😎 Deploy Preview https://deploy-preview-767--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.

Copy link
Contributor

@j08lue j08lue left a comment

Choose a reason for hiding this comment

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

Awesome to see these improvements! 😍

A few issues I discovered:

1. Timelime zoom buttons should be shown by default

The buttons currently only appear when there is an active analysis. They are also useful when just viewing data coverage, so they should be always visible.

image

2. Timeline zoom-in button faster than zoom-out

Pressing zoom-in (+) seems to jump two increments, where zoom-out (-) jumps one.

3. Timeline zoom increments could be smaller in the long time range (years)

Steps to reproduce

  1. Add multi-year dataset (e.g. NO2 PT)
  2. Run an analysis for some area over a 2-year time period
  3. Zoom all the way out to multi-year view
  4. Try to use the timeline zoom slider or +/- buttons to find a view that nicely fits the time series

I found this hard, because the first 1-2 zoom increments take me from full-domain 8-year view to roughly 1-year view, which is too close to view my 2-year time series. It would be great if the first zoom increments did not make that big a jump.

I know this will be hard to optimize for every possible dataset and analysis length combination. But multi-year datasets are quite common and that users will want to slowly zoom in from full-domain view, so perhaps we could try to improve this end a little?

Idea: every increment halves the domain length. Perhaps keeping it centered on the playhead, when possible.

4. Some icons / buttons are missing tooltips

  • image - Hide y-axes / Show y-axes (?)
  • image - Dataset description | Layer controls | Show layer / Hide layer
  • image - Select layer date
  • image - Select date range for analysis
  • (acknowledging that map zoom icons cannot currently get them for technical reasons)

@faustoperez
Copy link

Great job implementing the changes @danielfdsilva , that was fast ⚡ Great solution for integrating the trash icon on the analysis options panels too! Love the new selection modal 👍

+1 to what @j08lue said, though I don't think we need the tooltips on the chevron-down icons which are part of the dropdown and a common pattern (the label is the date in this case). I would also rename the expand icons to "Expand charts" "Collapse charts"

These are other small things I found:

  • I don't think we need the alt behaviour if we are showing the tooltip icons, there's a duplicity now
image
  • I think we can safely remove the layer count here, it makes more sense in the dataset selection modal.
image

@faustoperez
Copy link

Add analysis banners -- add primary button to start analysis ❌
This is still being decided (right @faustoperez?)

@danielfdsilva The designs are ready but I'd like to discuss their technical feasibility, any time that works on your side :)

@danielfdsilva
Copy link
Collaborator Author

@j08lue Comments addressed. I made the zoom halve the width as you suggested. It looks good but the slider jumps are not constant (as expected), but looks somewhat strange... still better than before IMO.

@faustoperez

  • Although I agree with the redundancy of tooltips on the date selectors I think they may be useful during the compare mode, and to clearly indicate the analysis range.
  • The alt behavior comes from the svg title. I can remove it in the custom icons, but collecticons will still have them, and can't be removed for now (known issue). So I left them everywhere.
  • Let's talk about the messages.
  • Removing the layer count, makes that area look very strange, even adjusting the padding. Have a loo and let me know.
image

@faustoperez
Copy link

Although I agree with the redundancy of tooltips on the date selectors I think they may be useful during the compare mode, and to clearly indicate the analysis range.
The alt behavior comes from the svg title. I can remove it in the custom icons, but collecticons will still have them, and can't be removed for now (known issue). So I left them everywhere.

Got it, thanks for the clarification!

Removing the layer count, makes that area look very strange, even adjusting the padding. Have a loo and let me know.

If we want to keep the subtitle there to make the block more balanced I suggest we change the text to "[number] layer(s) added." It still would give you the number of active layers so people know they have to scroll, which I think is the purpose of the subtitle. What I find a bit confusing about putting the total number of available layers there is that people won't necessarily know what that total number is referring to. That is my assumption, what do y'all think? 🤔

Let's talk about the messages.

Great, I'll book a session for this week 👍

@danielfdsilva
Copy link
Collaborator Author

@faustoperez great suggestion. Done

Copy link
Contributor

@j08lue j08lue left a comment

Choose a reason for hiding this comment

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

Most of previous review addressed, thanks!

Linearizing the timeline zoom slider

I made the zoom halve the width as you suggested. It looks good but the slider jumps are not constant (as expected), but looks somewhat strange...

It would actually work a lot better if the slider was moving linearly (stepping from 0 to n in steps of 1), but this would translate into powers of two for scaling the time domain.

Like $s = 2^i$ where $s$ is the scale factor and $i$ is the position on the zoom slider scale - or the inverse $i = log_2(s)$ when converting from an arbitrary scale factor to a position on the zoom slider. 🤷

https://github.com/NASA-IMPACT/veda-ui/pull/767/files#diff-1ecfd7065b69debceb2b45a7bbb91e0ce875d19d2bea91c24dd873a02d3ce93cR41-R70

Maybe using a D3 log₂ scale can do the same?

This would mean it feels linear when you drag the zoom slider or step through zoom levels with +/-, even though the actual time lone scaling happens in powers of two. It would also make the slider equally sensitive to dragging in the lower and higher ranges.

No, I think this is not overengineering. 😄

Side-by-side comparison falsely causes Analysis state to be Outdated

Issue: When you have an active analysis and activate the side-by-side comparison, the analysis state changes to "Outdated", even though none of the analysis parameters (time range, AOI) have changed.

Steps to reproduce:

  1. Add a dataset
  2. Complete an analysis
  3. Activate the side-by-side comparison
  4. Analysis state is now "Outdated", even though analysis-relevant parameters did not change.

New issue: AOI is not active after being drawn

Steps to reproduce:

  1. Add a dataset
  2. Draw an AOI
  3. AOI is not selected and message says I have to select one before I can start an analysis

Expected behavior would be that the AOI I just drew is selected by default.

New issue: AOI flickers while analysis results are loading

While analysis results are loading, the AOI on the map seems to flicker - quickly changing color or other kind of style intermittently.

Also happens when applying changes after changed parameters.

@danielfdsilva
Copy link
Collaborator Author

Log scale... Love it ❤️

Compare and analysis: Actually I think that this should be disabled. You can't compare 2 analysis and this is a bit misleading. Thoughts @j08lue @faustoperez ?
AOI is not active after being drawn: fixed
Flickering: This is something that was already happening. Will need to investigate-

@faustoperez
Copy link

faustoperez commented Nov 29, 2023

Compare and analysis: Actually I think that this should be disabled. You can't compare 2 analysis and this is a bit misleading. Thoughts @j08lue @faustoperez ?

Agree! I think those two "modes" should be mutually exclusive. But analysis should be persistent, i.e. when exiting comparison mode, if there was an analysis running, the app should remember its previous state. What do y'all think?

@j08lue
Copy link
Contributor

j08lue commented Nov 29, 2023

Actually I think that this should be disabled. You can't compare 2 analysis and this is a bit misleading.

I could actually see the combination to be useful: Zonal statistics time series analysis might point out two interesting states of the system - e.g. a pronounced positive and a negative excursion - and then you can use the slider to compare the two. Our toy task for users was to look at peak fire activity over an area and see whether the spatial patterns were the same between seasons. But you can do that by switching between points in time, too, instead of slide-comparison. And this was only a toy example...

So, we could disable this duality for now, to keep it simple, and see whether any (test) user ever mentions it - or ask them directly at some point.

@j08lue
Copy link
Contributor

j08lue commented Nov 29, 2023

I loove the smooth time line zooming now, @danielfdsilva, awesome work! ✨😍

Only things left from my previous review, for the record:

  • Disable comparison when analysis is active (as per call above)
  • Fix AOI flickering during analysis loading

@faustoperez
Copy link

I could actually see the combination to be useful: Zonal statistics time series analysis might point out two interesting states of the system - e.g. a pronounced positive and a negative excursion - and then you can use the slider to compare the two.

I see your point, the tricky part then would be to make the comparison slider work with the analysis menus and states 🤔 . I will take another look and report back

@danielfdsilva
Copy link
Collaborator Author

@j08lue Fix reported problems, and introduced a new one.

The flickering was happening because the analysis would update the dataset layers, which would cause the map style to be updated. This map style is created by our generators, which were not contemplating the map layers added by the aoi tool.
So, our style generators removed the aoi layers, just for the aoi to add them again. This removing/adding caused the flickering.

The solution I implemented is for the style generator, not to touch (read delete) any layer that was not created by it. This solves the flickering because the layers are not removed.

However, it introduced a new bug: When enabling the compare map, the style applied to it, is the same from the main map, which means that the aoi layers will also be there. The aoi plugin is then initialized on the compare map and it tries to add all its layers, causing an Error (There is already a source with this ID). It is normal for a plugin to do a setup on initialization, and in 99% of the times a plugin doesn't need to check if its own layers are already there on initialization, but in this case would have been handy...
Besides an error in the console, this has no other impact as far as I can tell.
I couldn't figure out how to solve this, and don't want to spend more time on in. If @hanbyul-here or @sandrahoang686 want to have a go, could be fun! (if you like this sort of thing 😅).

Just wanted to log this for the future. Not sure how much you want to keep track of this.

Copy link
Contributor

@j08lue j08lue left a comment

Choose a reason for hiding this comment

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

Great, the flickering is gone. All complete for the scope of this PR and review! ✨

@j08lue
Copy link
Contributor

j08lue commented Dec 1, 2023

Besides an error in the console, this has no other impact as far as I can tell.

We can live with that. Might this go away when we refactor the Mapbox draw thing (#710)?

@danielfdsilva
Copy link
Collaborator Author

We can live with that. Might this go away when we refactor the Mapbox draw thing (#710)?

It runs a little deeper than that...

Will merge. Great back and forth on this PR 🎉

@danielfdsilva danielfdsilva merged commit ab4578b into feature/exploration Dec 4, 2023
8 checks passed
@danielfdsilva danielfdsilva deleted the feature/exloration-nov27 branch December 4, 2023 09:20
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.

3 participants