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(explorer): collapsible mobile explorer #1471

Open
wants to merge 25 commits into
base: v4
Choose a base branch
from

Conversation

saberzero1
Copy link
Collaborator

@saberzero1 saberzero1 commented Oct 1, 2024

Screencast.from.2024-10-01.23-34-59.webm

Progress

  • Mobile explorer fully functional
    • Collapsible
    • Icon mode on mobile
  • Verify desktop explorer functionality
  • Restore styles
    • Desktop layout
    • Full screen mobile layout
  • Clean up
  • Update documentation
  • Restore default configuration

@saberzero1 saberzero1 mentioned this pull request Oct 1, 2024
@saberzero1 saberzero1 marked this pull request as ready for review October 1, 2024 23:02
@saberzero1
Copy link
Collaborator Author

@aarnphm I suppose this is another PR with a breaking change, as the Explorer component is no longer DesktopOnly. Keeping it DesktopOnly keeps the functionality the same as currently, but makes the mobile Explorer view hidden by default.

@saberzero1 saberzero1 mentioned this pull request Oct 1, 2024
7 tasks
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

cc jacky on this.

quartz.layout.ts Show resolved Hide resolved
quartz/components/Explorer.tsx Outdated Show resolved Hide resolved
quartz/components/Explorer.tsx Outdated Show resolved Hide resolved
quartz/components/scripts/explorer.inline.ts Outdated Show resolved Hide resolved
quartz/components/scripts/explorer.inline.ts Outdated Show resolved Hide resolved
@aarnphm aarnphm requested a review from jackyzha0 October 2, 2024 03:44
@saberzero1
Copy link
Collaborator Author

cc jacky on this.

Thanks for the review.

I think most of your feedback is valid. I'll do some more experimenting to reduce redundant code/hacks.

I have another implementation that is a lot more sophisticated, but requires the navigation tree to be included twice. I remember that this is something we explicitly want to avoid, mostly due to accessibility concerns.

I'll ping you once I am satisfied with my changes.

@saberzero1
Copy link
Collaborator Author

@aarnphm I believe I have addressed your feedback.

Once the code changes are approved, I'll update the documentation and restore the default configuration (DesktopOnly on Explorer).

Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

cc jacky on this as well 😄

quartz/components/Explorer.tsx Outdated Show resolved Hide resolved
quartz/components/ExplorerNode.tsx Outdated Show resolved Hide resolved
quartz/components/Footer.tsx Outdated Show resolved Hide resolved
quartz/components/ExplorerNode.tsx Outdated Show resolved Hide resolved
@Stephen-X
Copy link
Contributor

Stephen-X commented Oct 14, 2024

Thanks for this!

I'm testing the mobile explorer in my setup. For reasons still unknown to me, the explorer stopped being scrollable in desktop view.

My left sidebar uses a grid layout so that the search bar and the dark mode toggle are on the same row, and the recent notes are pinned to the page bottom.

This is the setting for the explorer in the middle; it has a variable height max-height: 100% as I want it to be collapsible.

.page .sidebar.left > .explorer-container {
    grid-area: 3/1/4/5;
    align-self: flex-start;
    min-height: 3.5rem;
    max-height: 100%;
}
Screenshot 2024-10-13 at 10 11 13 PM

With this change, unless I set a concrete size to the max-height of explorer-content, using max-height: 100% no longer shows the scrollbar, even though overflow-y: auto. I haven't quite figured out yet how to fix this.

@saberzero1
Copy link
Collaborator Author

Thanks for this!

I'm testing the mobile explorer in my setup. For reasons still unknown to me, the explorer stopped being scrollable in desktop view.

My left sidebar uses a grid layout so that the search bar and the dark mode toggle are on the same row, and the recent notes are pinned to the page bottom.

This is the setting for the explorer in the middle; it has a variable height max-height: 100% as I want it to be collapsible.

.page .sidebar.left > .explorer-container {
    grid-area: 3/1/4/5;
    align-self: flex-start;
    min-height: 3.5rem;
    max-height: 100%;
}
Screenshot 2024-10-13 at 10 11 13 PM

With this change, unless I set a concrete size to the max-height of explorer-content, using max-height: 100% no longer shows the scrollbar, even though overflow-y: auto. I haven't quite figured out yet how to fix this.

Can you link the page in question?

@Stephen-X
Copy link
Contributor

Stephen-X commented Oct 14, 2024

...
My left sidebar uses a grid layout so that the search bar and the dark mode toggle are on the same row, and the recent notes are pinned to the page bottom.
This is the setting for the explorer in the middle; it has a variable height max-height: 100% as I want it to be collapsible.

.page .sidebar.left > .explorer-container {
    grid-area: 3/1/4/5;
    align-self: flex-start;
    min-height: 3.5rem;
    max-height: 100%;
}

...
With this change, unless I set a concrete size to the max-height of explorer-content, using max-height: 100% no longer shows the scrollbar, even though overflow-y: auto. I haven't quite figured out yet how to fix this.

Can you link the page in question?

Here's a simple repro: Stephen-X@206f8bc. Feel free to pull my branch, it's just serving Quartz doc as test content plus a190cba.

I think you merely introduced an additional explorer-container wrapper here, so I'm not sure yet what fixed the height that caused explorer-content to not be scrollable. Tested several variations, but only setting height to a fixed size worked :(

@saberzero1
Copy link
Collaborator Author

...
My left sidebar uses a grid layout so that the search bar and the dark mode toggle are on the same row, and the recent notes are pinned to the page bottom.
This is the setting for the explorer in the middle; it has a variable height max-height: 100% as I want it to be collapsible.

.page .sidebar.left > .explorer-container {
    grid-area: 3/1/4/5;
    align-self: flex-start;
    min-height: 3.5rem;
    max-height: 100%;
}

...
With this change, unless I set a concrete size to the max-height of explorer-content, using max-height: 100% no longer shows the scrollbar, even though overflow-y: auto. I haven't quite figured out yet how to fix this.

Can you link the page in question?

Here's a simple repro: Stephen-X@206f8bc. Feel free to pull my branch, it's just serving Quartz doc as test content plus a190cba.

I think you merely introduced an explorer-container here, so I'm not sure yet what fixed the height that caused explorer-content to not be scrollable. Tested several variations, but only setting height to a fixed size worked :(

I see. That is just a flex thing. Any height should work. Try height: 4px;.

@Stephen-X
Copy link
Contributor

Stephen-X commented Oct 14, 2024

I see. That is just a flex thing. Any height should work. Try height: 4px;.

Yes, it's nested flex element having problem figuring out the parent flex element's height. Unfortunately to make scrolling available, I'll have to set the following line to a fixed size (e.g. 100vh) rather than 100%, otherwise the mobile explorer stopped being scrollable.

@saberzero1
Copy link
Collaborator Author

saberzero1 commented Oct 14, 2024

I see. That is just a flex thing. Any height should work. Try height: 4px;.

Yes, unfortunately to make scrolling available, I'll have to set the following line to a fixed size rather than 100%, otherwise the mobile explorer stopped being scrollable.

Oh I think this is a simple enough thing to fix, it's nested flex element having problem figuring out the parent flex element's height. I can override the above to height: 100vh to fix this; downside is scrollbar is truncated so probably can't ask that you change it in your PR :)

You want me to make a change in order to fix your custom changes?

I'll see if I can clean up the styling, but the explorer should always work on the default Quartz.

If you want to fix your left sidebar without a grid layout, consider something like this

@Stephen-X
Copy link
Contributor

Stephen-X commented Oct 14, 2024

You want me to make a change in order to fix your custom changes?

I'll see if I can clean up the styling, but the explorer should always work on the default Quartz.

If you want to fix your left sidebar without a grid layout, consider something like this

Thanks, but I'm not sure if changing height to any thing other than 100% would be ideal in the official code, that's why I deleted my comment; ideally explorer should take up as much height as possible, folks should override if they want otherwise.

Yes I might go with your flex-based setup if this is merged. On second thought this may not be related to flex at all. For overflow to work, must avoid having nested containers with height set to a percentage of the parent container's height which is also a percentage of its parent's.

Thanks for your help! The mobile explorer may be a breaking change for grid-based setup, a major version bump might be needed?

@saberzero1
Copy link
Collaborator Author

You want me to make a change in order to fix your custom changes?

I'll see if I can clean up the styling, but the explorer should always work on the default Quartz.

If you want to fix your left sidebar without a grid layout, consider something like this

Thanks, but I'm not sure if changing height to any thing other than 100% would be ideal in the official code, that's why I deleted my comment; ideally explorer should take up as much height as possible, folks should override if they want otherwise.

Yes I might go with your flex-based setup if this is merged. On second thought this may not be related to flex at all. For overflow to work, must avoid having nested containers with height set to a percentage of the parent container's height which is also a percentage of its parent's.

Thanks for your help! The mobile explorer may be a breaking change for grid-based setup, a major version bump might be needed?

The sidebars are display: flex; by default. I'm not sure if I understand how this would be a breaking change?

@Stephen-X
Copy link
Contributor

Stephen-X commented Oct 14, 2024

The sidebars are display: flex; by default. I'm not sure if I understand how this would be a breaking change?

Debugging your website, I just realized there isn't an .explorer-container wrapper in the solution you committed there: https://github.com/saberzero1/quartz/blob/3a47df05e36c120aaa8f094e4f6c2be728c8b0a2/quartz/components/ExplorerBurger.tsx, maybe that's why scrollbar is showing up?

I think the problem is browsers can't calculate height correctly for overflow if there are say 3 or more nested elements with height set as a percentage of their parent's, not sure yet if it's related to them being flex containers or not (MDN doesn't state this clearly so I'm not 100% sure if the statement is true: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow#description).

If the solution can keep only the original 2 layers (.explorer{ #explorer-content{} }), then all behavior would stay the same way regardless of how forks write their custom layout?

@saberzero1
Copy link
Collaborator Author

The sidebars are display: flex; by default. I'm not sure if I understand how this would be a breaking change?

Debugging your website, I just realized there isn't an .explorer-container wrapper in the solution you committed there: https://github.com/saberzero1/quartz/blob/3a47df05e36c120aaa8f094e4f6c2be728c8b0a2/quartz/components/ExplorerBurger.tsx, maybe that's why scrollbar is showing up?

I think the problem is browsers can't calculate height correctly for overflow if there are say 3 or more nested elements with height set as a percentage of their parent's, not sure yet if it's related to them being flex containers or not (MDN doesn't state this clearly so I'm not 100% sure if the statement is true: https://developer.mozilla.org/en-US/docs/Web/CSS/overflow#description).

If the solution can keep only the original 2 layers (.explorer{ #explorer-content{} }), then all behavior would stay the same way regardless of how forks write their custom layout?

There is a reason for the change. It is needed to support mobile exporer without duplicating the entire explorer tree content. My own site is disregarding that requirement by just including it twice.

I'll look into making changes to this PR. I'll keep you posted.

@saberzero1 saberzero1 mentioned this pull request Oct 24, 2024
@saberzero1
Copy link
Collaborator Author

Alright, I have an idea. The explorer doesn't have to expand on mobile, or can just slide it.

@saberzero1
Copy link
Collaborator Author

@Stephen-X Hey, I have a new proposal for the mobile explorer. I want to slide it in from the side instead of dropping down/expanding. See below:

Screencast.from.2024-10-25.18-33-46.webm

We're currently debating between making the top part sticky, so that it stays on top as the user scrolls down, similar to my own site. In the clip above this is not the case.

If we were to make the top part sticky, it should probably be an option to configure in quartz.layout.ts or quartz.config.ts, and likely a separate change shortly after this PR is deployed.

@Stephen-X
Copy link
Contributor

Stephen-X commented Oct 26, 2024

@Stephen-X Hey, I have a new proposal for the mobile explorer. I want to slide it in from the side instead of dropping down/expanding. See below:

Screencast.from.2024-10-25.18-33-46.webm
We're currently debating between making the top part sticky, so that it stays on top as the user scrolls down, similar to my own site. In the clip above this is not the case.

If we were to make the top part sticky, it should probably be an option to configure in quartz.layout.ts or quartz.config.ts, and likely a separate change shortly after this PR is deployed.

I think either looks nice (either sliding in from the left or from the top), although in the case of left sliding-in, it might look better if the hamburger menu rotates counterclockwise to go with the slide-in animation like a knob 😄

I personally made the left sidebar sticky as well in mobile mode, wrote a plugin to auto-hide on scrolling 😆 Can contribute a PR if there's interest for a mobile title bar in the main repo. Here's my website: https://stepht.org/


If the solution can keep only the original 2 layers (.explorer{ #explorer-content{} }), then all behavior would stay the same way regardless of how forks write their custom layout?

There is a reason for the change. It is needed to support mobile exporer without duplicating the entire explorer tree content. My own site is disregarding that requirement by just including it twice.

I'll look into making changes to this PR. I'll keep you posted.

Regarding the reported issue with the extra explorer-container making the file list non-scrollable when the container itself has a max-height: 100% set on it in custom layout (as was also the case in your personal website setup last time I checked), I just did a quick test, I'm able to remove the extra explorer-container without much of an issue. Here's a sample commit (please note that the css part is crude, I mostly just moved everything out of .explorer-container; might need to move some stuff into .explorer): Stephen-X@76187d6.

And now the file list is scrollable again in desktop mode, confirming my theory before about overflow:

Screenshot 2024-10-25 at 11 01 52 PM

Here's what it looks like in mobile mode, I made some small adjustments to styling to make the icon vertically centered again after explorer-container is removed, see my notes in explorer.scss:

Screenshot 2024-10-25 at 11 01 06 PM

I'm not entirely sure what you were referring to regarding "duplicating the entire explorer tree content" if the container is removed. Please feel free to refer to the commit to see if there's anything breaking with the change.

With explorer-container removed the mobile explorer feature becomes a (mostly) non-breaking change to fork's existing custom layouts or styles on .explorer.

Btw, I also changed display: flex to display:inline-block for .explorer / .explorer-container, flex is shrinking the hamburger menu icon when there isn't enough space in the left sidebar, making it harder to tap on; also that's different behavior from other components.

@saberzero1
Copy link
Collaborator Author

Screencast.from.2024-10-26.17-58-29.webm

@aarnphm aarnphm self-requested a review October 26, 2024 17:25
}
}
}

.explorer {
display: flex;
Copy link
Contributor

@Stephen-X Stephen-X Nov 2, 2024

Choose a reason for hiding this comment

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

Might be better showing explorer as inline-block.

As reported, flex shrinks if there isn't enough space. So if the title bar becomes squeezy the explorer icon will become smaller, making it harder to tap on and also impacting explorer-content alignment, as demoed below:

Before

Screenshot 2024-11-02 at 2 26 46 AM

After

Screenshot 2024-11-02 at 2 36 51 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you verify this is not related to your use of a customized grid layout for your left sidebar over Quartz's default flex for the left sidebar?

Copy link
Contributor

@Stephen-X Stephen-X Nov 2, 2024

Choose a reason for hiding this comment

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

I didn't alter the flex layout in mobile mode.

Screenshot 2024-11-02 at 3 00 05 AM

I think this is easily reproducible if you make the title bar more squeezy by having a longer page title or use "responsive" dimension in Chromium browsers and make the page narrower. It's really related to the explorer container itself being a flex container causing its containing component (the hamburger button) to shrink.

Screenshot 2024-11-02 at 3 01 37 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I'll look into it.

I asked because your previous suggested change didn't work in default Quartz.

I'll let you know my conclusion soon.

Once again, thanks for the reports!

@Stephen-X
Copy link
Contributor

Bug report: Because explorer is hidden in folder index pages, the mobile explorer button also disappears from title bar.

@saberzero1
Copy link
Collaborator Author

Bug report: Because explorer is hidden in folder index pages, the mobile explorer button also disappears from title bar.

Can you check if this stil l occurs when the explorer is added to listpages in quartz.layout.ts?

@Stephen-X
Copy link
Contributor

Stephen-X commented Nov 2, 2024

Bug report: Because explorer is hidden in folder index pages, the mobile explorer button also disappears from title bar.

Can you check if this stil l occurs when the explorer is added to listpages in quartz.layout.ts?

Sorry that was a careless bug report. My setup mistake while testing the PR in different cases. I forgot I made explorer desktop-only for ListPages before this PR because the whole explorer would otherwise appear in the title bar.

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