feat(theme-default): hide aside element when no heading present

MaxtuneLee commented 2 months ago

Summary

Related Issue

closes: #915

Checklist

[ ] Tests updated (or not required).
[ ] Documentation updated (or not required).

CLAassistant commented 2 months ago

All committers have signed the CLA.

netlify[bot] commented 2 months ago

Deploy Preview for aquamarine-blini-95325f ready!

Name	Link
Latest commit	f2694bd68148d2117d960016795783a6cfc971f1
Latest deploy log	https://app.netlify.com/sites/aquamarine-blini-95325f/deploys/662e44ca585855000829c06d
Deploy Preview	https://deploy-preview-1031--aquamarine-blini-95325f.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 91 (🔴 down 2 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 92 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Timeless0911 commented 2 months ago

We'd better unified all logic that controls UI presentation in this function.

https://github.com/web-infra-dev/rspress/blob/70ebf18fc65ddf40715530767f72d490370446d9/packages/theme-default/src/logic/useUISwitch.ts#L18

MaxtuneLee commented 2 months ago

We'd better unified all logic that controls UI presentation in this function.

https://github.com/web-infra-dev/rspress/blob/70ebf18fc65ddf40715530767f72d490370446d9/packages/theme-default/src/logic/useUISwitch.ts#L18

get it 👌

Timeless0911 commented 2 months ago

Your alumni @9aoy

MaxtuneLee commented 2 months ago

Your alumni @9aoy

haha, what a surprise 😮❕︎

MaxtuneLee commented 2 months ago

We'd better unified all logic that controls UI presentation in this function.

https://github.com/web-infra-dev/rspress/blob/70ebf18fc65ddf40715530767f72d490370446d9/packages/theme-default/src/logic/useUISwitch.ts#L18

I've moved all the logic to useUISwitch, thanks for the review

Timeless0911 commented 2 months ago

I think add page.toc.length > 0 && is enough for this PR. The scene of no sidebar data already deal well?

MaxtuneLee commented 2 months ago

I think add page.toc.length > 0 && is enough for this PR. The scene of no sidebar data already deal well?

with sidebarData.items.length > 0

without sidebarData.items.length > 0

🤔️，I added that because I found that Object.keys(sidebar).length > 0 is true even if there is no sidebar in current page

Timeless0911 commented 2 months ago

I can not reproduce it.

My case is e2e/fixtures/basic

MaxtuneLee commented 2 months ago

I can not reproduce it.

My case is e2e/fixtures/basic

It works perfectly when there is nothing else, but when we add more pages like

then we can reproduce this issue I think this is because localesData.sidebar returns all sidebars in different pages by its path

Timeless0911 commented 2 months ago

I thought about it for a while and refered to the layout of Vitepress in such a scene that without headings in a doc.

The logic of this judgment is to reserve the placeholder sidebar and outline aside for the entire document. This is beneficial to the overall layout of the document and also prevents the feeling of transition when switching each page in the document.

At the same time, there are may be some height calculations logic in document layout that require dynamic layout by relying on some placeholder width.

For a special single page, we are more suitable to use FrontMatter to control the display of related components for a separate page.

我想了一下，并参考了 Vitepress 在没有标题时候的布局。

这样判断的逻辑让文档的整体保留占位的 sidebar 和 outline aside，对于文档整体的布局是有益的，它使得文档中每个页面切换时不会产生瞬移的感觉。

同时，文档布局中可能还有一些高度的计算，需要依靠一些占位的模块来进行动态布局。

对于单页面的情况，我们更加适合使用 FrontMatter 为单独的页面控制相关组件的展示。

@linbudu599 @MaxtuneLee cc What's your opinion?

MaxtuneLee commented 2 months ago

I thought about it for a while and refered to the layout of Vitepress in such a scene that without headings in a doc.

The logic of this judgment is to reserve the placeholder sidebar and outline aside for the entire document. This is beneficial to the overall layout of the document and also prevents the feeling of transition when switching each page in the document.

At the same time, there are may be some height calculations logic in document layout that require dynamic layout by relying on some placeholder width.

For a special single page, we are more suitable to use FrontMatter to control the display of related components for a separate page.

我想了一下，并参考了 Vitepress 在没有标题时候的布局。

这样判断的逻辑让文档的整体保留占位的 sidebar 和 outline aside，对于文档整体的布局是有益的，它使得文档中每个页面切换时不会产生瞬移的感觉。

同时，文档布局中可能还有一些高度的计算，需要依靠一些占位的模块来进行动态布局。

对于单页面的情况，我们更加适合使用 FrontMatter 为单独的页面控制相关组件的展示。

@linbudu599 @MaxtuneLee cc What's your opinion?

I think ur right, reserving sidebar by default make page transition smoother, and we also have sidebar option to let users choose what they want, from overall consideration, I think this is reasonable to keep the sidebar.

chenjiahan commented 2 months ago

The logic of this judgment is to reserve the placeholder sidebar and outline aside for the entire document. This is beneficial to the overall layout of the document and also prevents the feeling of transition when switching each page in the document.

Agreed 👍

Timeless0911 commented 2 months ago

I will close this PR and its associated issues soon, thanks for your attention.

web-infra-dev / rspress