Skip to content

improve: Home button in notebook menu#1997

Merged
akshayka merged 6 commits into
marimo-team:mainfrom
wasimxyz:improvement/home-button
Aug 12, 2024
Merged

improve: Home button in notebook menu#1997
akshayka merged 6 commits into
marimo-team:mainfrom
wasimxyz:improvement/home-button

Conversation

@wasimxyz

@wasimxyz wasimxyz commented Aug 10, 2024

Copy link
Copy Markdown

📝 Summary

Completes #1984. Adds home button at top left of open notebook page next to app config and shutdown buttons.

Also updates "Running notebooks" section on home page to open running notebooks in the same tab for navigation consistency.

🔍 Description of Changes

Screenshot 2024-08-10 at 3 30 45 PM

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@mscolnick

@vercel

vercel Bot commented Aug 10, 2024

Copy link
Copy Markdown

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

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2024 5:10pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2024 5:10pm

@akshayka akshayka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! Two pieces of feedback.

  1. The home button is styled differently than the NotebookMenuDropdown and Config buttons — can you update the style of the home button to match the NotebookMenuDropdown and Config buttons?

  2. When a notebook is started with marimo new or marimo edit notebook.py, we don't serve the home page — in which case the home button is just refreshing the notebook, which is confusing. I suggest only showing the home button when we're actually serving the home page.

{!closed && (
<div className={topRightControls}>
{presenting && <LayoutSelect />}
<HomeButton />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: I suggest putting HomeButton after ConfigButton.

@wasimxyz

wasimxyz commented Aug 10, 2024

Copy link
Copy Markdown
Author

Ah I didn't account for marimo new and marimo edit filename.py. Is there some flag I can use somewhere, or would it be okay to infer that a home page wasn't rendered by checking whether the URL has query parameters?

Looks like both of these commands open a notebook at https://domain.com/, while opening a notebook from home does "https://domain.com/?file=".

@akshayka akshayka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Iterating on the design — I think it makes most sense for the home to be an action in the hamburger menu, right above "restart notebook", with title "Return home". Can you make that change?

image

@wasimxyz

wasimxyz commented Aug 11, 2024

Copy link
Copy Markdown
Author

Sure! Personally I think a navigation item might get lost in the hamburger menu (would a user intuitively open a menu to return home?), but I'm not particularly opinionated about this

@akshayka

Copy link
Copy Markdown
Contributor

Sure! Personally I think a navigation item might get lost in the hamburger menu (would a user intuitively open a menu to return home?), but I'm not particularly opinionated about this

The home action I view as a convenience, not an integral part of the top-bar, so I'd like to emphasize it less.

@wasimxyz

wasimxyz commented Aug 11, 2024

Copy link
Copy Markdown
Author
Screenshot 2024-08-11 at 10 08 02 AM

🙂

@akshayka

Copy link
Copy Markdown
Contributor
Screenshot 2024-08-11 at 10 08 02 AM 🙂

Fantastic thank you!

divider: true,
icon: <Home size={14} strokeWidth={1.5} />,
label: "Return home",
hidden: !location.search.includes("file"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mscolnick can you comment whether this is an acceptable way to check whether the server is capable of serving the "home" page?

@mscolnick mscolnick Aug 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yea this is fine. Potentially in a future change, we should allow going back home regardless, and just use the current directory for "marimo new"

@wasimxyz

Copy link
Copy Markdown
Author

Seems as though a test is failing with Python 3.11, is it just a fluke?

@akshayka

Copy link
Copy Markdown
Contributor

Seems as though a test is failing with Python 3.11, is it just a fluke?

Yea don't worry about the failure, it's unrelated, I'm handling it in another PR.

@wasimxyz wasimxyz changed the title improve: Home button in notebook toolbar improve: Home button in notebook menu Aug 12, 2024

@akshayka akshayka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

@akshayka akshayka merged commit eff559f into marimo-team:main Aug 12, 2024
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.7.20-dev11

@wasimxyz wasimxyz deleted the improvement/home-button branch August 12, 2024 19:20
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