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

Site Sider #162

Merged
merged 67 commits into from
Jun 19, 2024
Merged

Site Sider #162

merged 67 commits into from
Jun 19, 2024

Conversation

SanjeevLakhwani
Copy link
Contributor

@SanjeevLakhwani SanjeevLakhwani commented May 16, 2024

  • Added a sider that replaces the tabbed dashboard in the previous version

@SanjeevLakhwani SanjeevLakhwani changed the title [WIP] Site sider Site Sider May 16, 2024
@SanjeevLakhwani SanjeevLakhwani self-assigned this May 16, 2024
@SanjeevLakhwani SanjeevLakhwani requested a review from gsfk May 16, 2024 19:45
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

needs to account for custom logos

remove extra beacon file

src/js/components/SiteSider.tsx Show resolved Hide resolved
src/styles.css Outdated Show resolved Hide resolved
Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

Side menu much prettier than the old tabs!

Perhaps a little late for this question, but what's the advantage of a dynamic "sider" over a simple side menu? I would think the main use case would be the ability to show a smaller menu when the sceen is smaller, but this pr doesn't do that.

Also, since tabs are gone the pages no longer have large text showing you which tab you're on... now you have only super-small text in the side menu to know where you are. Not sure if pages need header text added or if I'm just not used to it yet.

@SanjeevLakhwani SanjeevLakhwani requested a review from gsfk May 22, 2024 20:34
Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

I'm not totally sold on the new header text... although the text appears in the same spot for each page, the page layout isn't consistent. Overview and Provenance are aligned with the component below them:

Screenshot from 2024-05-23 10-15-24

... but Search & Beacon are not:
gap

I also just find them ugly... I know that's not a particularly helpful comment. I wonder if this text wouldn't be better off in the header? Not sure.

header

@SanjeevLakhwani SanjeevLakhwani requested a review from gsfk May 29, 2024 16:28
Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

aesthetically, I think the bright white header text/button outlines are bit too harsh on the dark header background, but the visual hierarchy with the new sidebar being context for what page/project/dataset we are on works quite well.

maybe the contents of the sidebar should be sticky, so that we can change pages even if scrolled-down?

weird horizontal overflow/scroll, only on overview page on small screen (14" laptop)
Screenshot 2024-06-03 at 4 04 06 PM

src/js/components/SiteSider.tsx Outdated Show resolved Hide resolved
src/js/hooks.ts Outdated Show resolved Hide resolved
src/js/hooks.ts Outdated Show resolved Hide resolved
src/js/hooks.ts Outdated Show resolved Hide resolved
src/styles.css Outdated Show resolved Hide resolved
Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

So....

  • top of the screen is black but side is white?
  • current tab indicated from menu only?

... I wasn't sure where we landed on either of these issues.

The "Search" page does not behave consistently: if you load the main page and click Search it shows an overview, but it you load /search it shows zero results.

Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

how will the new file for branding work with our existing instances that replace the logo?

@SanjeevLakhwani
Copy link
Contributor Author

how will the new file for branding work with our existing instances that replace the logo?

I corrected the filenames to match the previous, so it should be the same as before

Copy link
Member

@davidlougheed davidlougheed left a comment

Choose a reason for hiding this comment

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

icon on both left and right of portal button - on left, the old icon; on right, the link icon. otherwise looks good i think.

Copy link
Member

@gsfk gsfk left a comment

Choose a reason for hiding this comment

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

The results pane jumps when switching between "Search" and "Beacon", it should be in the same place.

@SanjeevLakhwani SanjeevLakhwani requested review from gsfk and davidlougheed and removed request for davidlougheed June 19, 2024 19:47
@SanjeevLakhwani SanjeevLakhwani merged commit b869674 into main Jun 19, 2024
2 checks passed
@davidlougheed davidlougheed deleted the site-sider branch July 15, 2024 17:51
@davidlougheed davidlougheed restored the site-sider branch July 15, 2024 17:51
@davidlougheed davidlougheed deleted the site-sider branch August 26, 2024 15:12
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