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

feat: New UI (search + dataset preview) #818

Merged
merged 14 commits into from
Nov 4, 2022
Merged

feat: New UI (search + dataset preview) #818

merged 14 commits into from
Nov 4, 2022

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Nov 1, 2022

Closes #817.

This PR introduces new UI for search and dataset preview pages.

@vercel
Copy link

vercel bot commented Nov 1, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
visualization-tool ✅ Ready (Inspect) Visit Preview Nov 4, 2022 at 9:24AM (UTC)

@bprusinowski
Copy link
Collaborator Author

@AnninaWalker can you take a look at deployment preview and see if dataset highlighting on mouse over looks good? Previously we had some left padding applied to the cards so there was some space on the left side, not sure if it looks ok now 😅

@ptbrowne
Copy link
Collaborator

ptbrowne commented Nov 2, 2022

Wow, I haven't looked at the code yet, but this looks really nice, kudos @AnninaWalker and @bprusinowski ! I love the header animation 💯

Concerning the hover, If you are talking about this @bprusinowski, I think you need to fix it 😁, I think you need to apply a cursor pointer also.

image

@bprusinowski
Copy link
Collaborator Author

Cool, thanks @ptbrowne! I am not 100% happy with the animation, as when you scroll down to select a dataset, it jumps to the top immediately and just after that it starts shrinking. I tried to "fix" this by animating the scroll up (as by default router.push scrolls to the top without the animation), but I wasn't sure if it looked good either. Let me know if it's fine this this :)

And for the hovering, I think the cursor should be there already, it's not a pointer on your side?

Screenshot 2022-11-02 at 17 10 05

@ptbrowne
Copy link
Collaborator

ptbrowne commented Nov 2, 2022

Ah sorry, for the pointer, I made a mistake, my focus was not on the window when I saw that.

I see your point concerning the animation when you have scrolled down, it's true that it's a bit distracting. Maybe we could try to deactivate the animation altogether when the banner is not in the viewport ?

@ptbrowne
Copy link
Collaborator

ptbrowne commented Nov 2, 2022

Nit: Animation is a bit annoying when switching language

LGTM at the moment for me 👍

@bprusinowski
Copy link
Collaborator Author

Ah, good point about switching the language, I missed that. I'll try with the viewport suggestion + think about locale switch 🙇

@AnninaWalker
Copy link
Contributor

YAY nice to see the new search layout in action and even animated! So cool! Regarding your questions @bprusinowski I think the highlighting of the dataset would need some padding. Maybe it could even extend to the line on the left?

@bprusinowski
Copy link
Collaborator Author

@AnninaWalker @ptbrowne I introduced the changes, I it should be better now :)

I also managed to fix the locale switching animation problem, but it means that we will lack initial banner animation. Let me know if it's acceptable 😅

@AnninaWalker
Copy link
Contributor

@bprusinowski LGTM. The only thing I was wondering is regarding the responsive behaviour, if the content shouldn't be centred?

1 3 22_l_ui-browse-datasets copy

@bprusinowski
Copy link
Collaborator Author

@AnninaWalker the whole site should "extend" to the available width, or the banner content should be centered (right now it's left-aligned to the content below)? Or should the banner take more space, to have the same width as the dataset results below?

@AnninaWalker
Copy link
Contributor

Currently the content below has at some point a fixed width and the content is aligned to the left, so when I extend my window, I get this white whole on the right. That's why I was wondering if we should center the content, as in the screenshot above. But maybe it doesn't make sense. I guess most people don't have such a huge screen as I do ;-)

Screenshot 2022-11-03 at 18 10 33

Copy link
Collaborator

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

👏

@bprusinowski bprusinowski merged commit 4a756f7 into main Nov 4, 2022
@bprusinowski bprusinowski deleted the feat/new-ui-1 branch November 4, 2022 12:42
@bprusinowski bprusinowski mentioned this pull request Nov 4, 2022
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.

New User Interface, search + preview
3 participants