-
Notifications
You must be signed in to change notification settings - Fork 6
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 page (E&A merge) #598
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really impressive work 👏
There are a few technical things that we may want to discuss further, otherwise I have some feedback on useability:
- Disable text selection when dragging datasets (on datasets names, etc);
- Users should be able to change L and R markers at the same time by dragging the inner partM
- TBH I'm not 100% sure what L, R and P markers do. And I think it could be useful for the letters to be explained, I mean I guess that L and R means Right and Left, but of what ? Could we use icons to aid understanding of this?
- The timeline whole area should be draggable (maybe it's already the case but I didn't find how);
- The whole timeline is zoomable, which is pretty awesome. However I found out about this by reading the code and performing an horizontal pinch on my trackpad with the Alt key pressed. I think we could use some improved discoverability of the feature;
- When selecting an L/R range that is displayed outside the current timeline viewport, it should automatically move the timeline viewport there;
- Should the date range picker automatically open by default on the start date of the timeline viewport, rather than today?;
width: 5rem; | ||
margin: 0 auto -1.25rem auto; | ||
padding: 0.25rem 0; | ||
z-index: 5000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there's a list defined in the theme. We just haven't been using it. 😢
veda-ui/app/scripts/styles/theme.ts
Lines 9 to 19 in 1ced268
zIndices: { | |
hide: -1, | |
docked: 10, | |
sticky: 900, | |
dropdown: 1000, | |
overlay: 1300, | |
modal: 1400, | |
popover: 1500, | |
skipLink: 1600, | |
toast: 1700, | |
tooltip: 1800 |
@@ -0,0 +1,472 @@ | |||
import React, { useEffect, useMemo, useRef, useState } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is getting on the larger side. Maybe consider starting with extracting away Controls
and DatasetList
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Still need to split some of the files
}); | ||
// Width of the whole timeline item. Set via a size observer and then used to | ||
// compute the different element sizes. | ||
export const timelineWidthAtom = atom<number | undefined>(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I could use a high-level explanation as to why this is part of the app state. Isn't that defined solely by the available witdh of the timeline viewport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several elements that need to know the width (specially the svg components), so I thought of storing the size in an atom and access it via a hook instead of having to pass width to every single component (this is something I still have to refactor) - Thoughts on the approach?
export default Timeline; | ||
|
||
function DatasetList(props: any) { | ||
const { datasets, ...rest } = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend against passing down datasets
, and directly use the atom value instead:
const { datasets, ...rest } = props; | |
const datasets = useAtomValue(timelineDatasetsAtom); |
IMHO it only makes sense to pass down a prop containing data when the component renders an element of an array, ie <Rabbit rabbitData={rabbits[i]} />
function DatasetList(props: any) { | ||
const { datasets, ...rest } = props; | ||
|
||
const [orderedDatasets, setOrderDatasets] = useState(datasets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of duplicating datasets and create potential syncing issues, why not altering the order of datasets on the atom directly?
); | ||
} | ||
|
||
export function TimelineHeadR(props: Omit<TimelineHeadProps, 'children'>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could wrap TimelineHeadP/L/R into a single component passing down the letter and the path object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several properties that are changing, not only the path (transform, text position, etc)
} from './constants'; | ||
|
||
// Datasets to show on the timeline and their settings | ||
export const timelineDatasetsAtom = atom<TimelineDataset[]>([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried playing with atomWithHash ?
Thank you for the great feedback items. There are a lot of accessibility improvements to be done to the timeline, and this is a great starting list. Some things will have to addressed via help information (like the zoom with
You only need to click and drag (once zoomed in, if you're viewing everything there's no dragging). Does this not work? 🤔 |
1bc7351
to
a0e284b
Compare
0d3d652
to
e1eef89
Compare
Includes opacity and dataset remove button
🤚 Have you considered integrating Jotai Devtool? https://jotai.org/docs/tools/devtools. Is there any specific reason you are not using it? |
@hanbyul-here Honestly I hadn't even seen them. 😅 Maybe could have been helpful. cc @j08lue |
Closes #770 - [x] Reduce steps to two - [x] Remove separate analysis tour - [x] Add visual cues / images to the steps - [ ] Offer a way for the user to invoke the tour again later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ghost time series value tooltip
After I "Exit Analysis", the time series values tooltip still seems to be active, even though the time series is not shown anymore in the bottom drawer. Steps to reproduce:
- Add a layer
- Run analysis - take note of where on the bottom drawer there are values
- Exit analysis
- Hover over bottom drawer over an area where there were time series values
- Issue: value tooltip is shown even though no time series is visible
@j08lue That was a cute one 😅. Fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j08lue Addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of my review comments (based on visual review) addressed! We are now asking for key stakeholder reviews on the preview deployment.
@@ -90,6 +89,7 @@ const prepareDatasets = ( | |||
d.name.toLowerCase().includes(searchLower) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be d.id.toLowerCase().includes(searchLower)
considering id is used to forward the user from the data overview page to this modal ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch @hanbyul-here! Funnily enough this was working well, but it was a coincidence. Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the comment https://github.com/NASA-IMPACT/veda-ui/pull/598/files#r1417895571
app/scripts/main.tsx
Outdated
@@ -56,6 +58,8 @@ const composingComponents = [ | |||
LayoutRootContextProvider | |||
]; | |||
|
|||
const useNewExploration = !!process.env.FEATURE_NEW_EXPLORATION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more suggestion to use an explicit value for this environment variable, but other than that I think this should be good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a simple function to be used in cases like this checkEnvFlag(process.env.VAR_NAME)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇 Thanks for working on this big feature. !
Thank you, Daniel! 🙌 |
Is PR is serving as a base for the development on the new Exploration and Analysis page. Branching in/out of this.
During development, the new page is accessible at
/exploration
.The sandbox code is still present but will be removed eventually. Disregard during reviews.
To Do
Implement line chart support in the dataset track #605
[E & A] - Analysis worflow #526
[E & A] - Structure of the Analysis/Exploration page #524
[E & A] - Date selector and timeline #522
Implement exploration page with timeline #580
[E & A] - Displaying selected datasets #521
[E & A] - Dataset comparison #523
[E & A] - First contact with page / initial exploration #520
[E & A] - Page status sharing #527
[E & A] Make high-fidelity time series plots more apparent #720
[E & A] Change play head and range selector look and behavior #721
[E & A] Make time line pan and zoom more user friendly #722
[E & A] Review analysis controls - start, reload, exit, ... #723
[E & A] Add option to retrieve WMTS and XYZ URLs for a layer #724
[E & A] Prototype and validate analysis workflow #579
Simplify feature tour #771
TBD