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.

Sign up for GitHub

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

Jump to bottom

fix: Make menu bar responsive #1059

Merged
merged 27 commits into from
Jul 30, 2024
Merged

fix: Make menu bar responsive #1059

merged 27 commits into from
Jul 30, 2024

Conversation

rainandbare
Copy link
Contributor

@rainandbare rainandbare commented Jul 25, 2024 •
edited by github-actions bot
Loading

RDEV LINK - https://pleasing-sturgeon.dev-sc.dev.czi.team/d/super-cool-spatial.cxg/?spatial=true

Fixes issue #1034

Preview

Screen.Recording.2024-07-25.at.1.27.00.PM.mov

@rainandbare rainandbare added stack and removed stack labels Jul 25, 2024
tihuan
tihuan approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 👏 Thanks so much for the clever solution, @rainandbare 🤩 🥇 🥇 🥇

client/src/components/menubar/index.tsx Outdated Show resolved Hide resolved
client/src/components/menubar/index.tsx Outdated Show resolved Hide resolved
client/src/components/menubar/index.tsx Outdated Show resolved Hide resolved
client/src/components/menubar/index.tsx Show resolved Hide resolved
hthomas-czi
hthomas-czi requested changes Jul 25, 2024
View reviewed changes
Copy link

@hthomas-czi hthomas-czi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Responsive behavior looks great but it has the same issue as the original implementation where the toolbar div prevents interaction with the underlying visualization if they overlap (see screenshot).

Screenshot 2024-07-25 at 1 36 07 PM

@tihuan
Copy link
Contributor

tihuan commented Jul 25, 2024

Responsive behavior looks great but it has the same issue as the original implementation where the toolbar div prevents interaction with the underlying visualization if they overlap (see screenshot).

Screenshot 2024-07-25 at 1 36 07 PM

Ah great catch, @hthomas-czi ! Sorry I missed this too 😭

@rainandbare rainandbare added stack and removed stack labels Jul 26, 2024
tihuan
tihuan approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

Looks super good! Just one non-blocking discussion 😄 Thanks so much again for the fantastic work, @rainandbare 🙏 ✨ !!

client/src/components/menubar/style.ts Show resolved Hide resolved
hthomas-czi
hthomas-czi approved these changes Jul 26, 2024
View reviewed changes
Copy link

@hthomas-czi hthomas-czi left a comment

Choose a reason for hiding this comment

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

Looks perfect! Thanks, @rainandbare

@rainandbare rainandbare removed the stack label Jul 26, 2024
tihuan
tihuan approved these changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

LGTM to the max 🚀 Thanks so much for the excellent solution, @rainandbare 🤩 🥇 🙏

client/src/components/menubar/style.ts Outdated Show resolved Hide resolved
client/src/components/menubar/style.ts Outdated Show resolved Hide resolved
client/src/components/menubar/style.ts Show resolved Hide resolved
client/src/components/menubar/index.tsx Outdated Show resolved Hide resolved
@tihuan
Copy link
Contributor

tihuan commented Jul 29, 2024

Hmm not sure why the e2e test is failing, since the menu bar change shouldn't affect it?

We can download the playwright test artifact at the test summary page and run something like npx playwright show-trace playwright-report/e2e-e2e-dataset-pbmc3k-cxg-graph-instance-layout-graph-clipping-clip-continuous-chromium/trace.zip to review how the test failed!

Screenshot 2024-07-29 at 7 57 22 AM

@rainandbare rainandbare enabled auto-merge (squash) July 30, 2024 14:04
@rainandbare
Copy link
Contributor Author

Hmm not sure why the e2e test is failing, since the menu bar change shouldn't affect it?

We can download the playwright test artifact at the test summary page and run something like npx playwright show-trace playwright-report/e2e-e2e-dataset-pbmc3k-cxg-graph-instance-layout-graph-clipping-clip-continuous-chromium/trace.zip to review how the test failed!

Screenshot 2024-07-29 at 7 57 22 AM

Phew - finally figured it out. The Clip dialog box was overlapping the test icon needed to get a screen capture in vertical mode.

@rainandbare rainandbare merged commit 2856e60 into main Jul 30, 2024
20 of 21 checks passed
@rainandbare rainandbare deleted the rainandbare/1034-responsive-menu-bar-2 branch July 30, 2024 18:16
@czi-github-helper czi-github-helper bot mentioned this pull request Jul 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hthomas-czi hthomas-czi hthomas-czi approved these changes

@tihuan tihuan tihuan approved these changes

@kaloster kaloster Awaiting requested review from kaloster

Assignees
No one assigned
Labels
stack
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rainandbare @tihuan @hthomas-czi