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 page (E&A merge) #598

Merged
merged 249 commits into from
Dec 11, 2023
Merged

Exploration page (E&A merge) #598

merged 249 commits into from
Dec 11, 2023

Conversation

@netlify
Copy link

netlify bot commented Aug 2, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit eb37147
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/6576e91539fe4a0008bfa7e9
😎 Deploy Preview https://deploy-preview-598--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

@nerik nerik left a 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be the subject of another PR, but I'd very much like for this project to have a set of z indices defined by the theme that we could stick to.
Screenshot 2023-08-03 at 14 01 07

Copy link
Collaborator Author

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. 😢

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';
Copy link
Contributor

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?

Copy link
Collaborator Author

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);
Copy link
Contributor

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?

Copy link
Collaborator Author

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;
Copy link
Contributor

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:

Suggested change
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);
Copy link
Contributor

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'>) {
Copy link
Contributor

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.

Copy link
Collaborator Author

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[]>([]);
Copy link
Contributor

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 ?

@danielfdsilva
Copy link
Collaborator Author

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 option|alt).

The timeline whole area should be draggable (maybe it's already the case but I didn't find how);

You only need to click and drag (once zoomed in, if you're viewing everything there's no dragging). Does this not work? 🤔

@hanbyul-here
Copy link
Collaborator

🤚 Have you considered integrating Jotai Devtool? https://jotai.org/docs/tools/devtools. Is there any specific reason you are not using it?

@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here Honestly I hadn't even seen them. 😅 Maybe could have been helpful.
Can we get a final review here to merge?

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?
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.

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:

  1. Add a layer
  2. Run analysis - take note of where on the bottom drawer there are values
  3. Exit analysis
  4. Hover over bottom drawer over an area where there were time series values
  5. Issue: value tooltip is shown even though no time series is visible
Screenshot 2023-12-06 at 13 20 17

@danielfdsilva
Copy link
Collaborator Author

@j08lue That was a cute one 😅. Fixed

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.

Datasets vs layers

Sorry - late find: We need to replace "dataset" with "layer" in the layer selection modal for now, please, until we introduce a new concept for datasets as layer groups.

image

@danielfdsilva
Copy link
Collaborator Author

@j08lue Addressed

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.

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) ||
Copy link
Collaborator

@hanbyul-here hanbyul-here Dec 6, 2023

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 ?

Copy link
Collaborator Author

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!

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

@@ -56,6 +58,8 @@ const composingComponents = [
LayoutRootContextProvider
];

const useNewExploration = !!process.env.FEATURE_NEW_EXPLORATION;
Copy link
Collaborator

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!

Copy link
Collaborator Author

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)

Copy link
Collaborator

@hanbyul-here hanbyul-here left a 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. :shipit: !

@danielfdsilva danielfdsilva merged commit 404043e into main Dec 11, 2023
8 checks passed
@danielfdsilva danielfdsilva deleted the feature/exploration branch December 11, 2023 17:17
@faustoperez
Copy link

Thank you, Daniel! 🙌

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.

5 participants