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

Support to set whether the sidebar will be shown in each single post. #93

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

tsanie
Copy link
Contributor

@tsanie tsanie commented Jan 28, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes have been added (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs have been added / updated (for bug fixes / features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

Now we couldn't control the sidebar will be shown in some posts and not be shown in the other ones.

Issue Number(s): #92

What is the new behavior?

If you've written a front-matter sidebar, then the sidebar will be controlled by it.

  • Screens with this changes: N/A
  • Link to demo site with this changes: https://tsanie.us/about/ (The sidebar will be shown even the page didn't have a TOC)

How to use?

In any post source file source/**/XXX.md:

...
date: 2018-01-28 21:16:00
sidebar: false
---

...

Does this PR introduce a breaking change?

  • Yes.
  • No.

@ivan-nginx
Copy link
Member

Don't und, why this? In short words.

@maple3142
Copy link
Contributor

I think it is because of #92.

@tsanie
Copy link
Contributor Author

tsanie commented Jan 28, 2018

@ivan-nginx Hide the specified page's sidebar in default (but in the config sidebar.display need to be 'post', for the other pages).

@ivan-nginx
Copy link
Member

Default to force true maybe?

@tsanie
Copy link
Contributor Author

tsanie commented Jan 28, 2018

Now, we could set the NexT config sidebar.display to 'hide', to force hide the sidebar.

But in order to the situation in #92 , I think we need a configure to set it for the each pages.

For example, now I need to set the sidebar.display to 'post' to shows the sidebar, when the post has a TOC. But in the meanwhile I need to hide the sidebar in another post, even it has the TOC.

@ivan-nginx
Copy link
Member

I see. Can u set var by default to true? For around possible bugs in future.

@@ -87,10 +87,15 @@ $(document).ready(function () {

// Expand sidebar on post detail page by default, when post has a toc.
var $tocContent = $('.post-toc-content');
var isSidebarCouldDisplay = CONFIG.sidebar.display === 'post' ||
var display = CONFIG.page.sidebar;
Copy link
Member

Choose a reason for hiding this comment

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

var display = true above this.

{# Exports some front-matter variables to Front-End #}
<script type="text/javascript" id="page.configurations">
CONFIG.page = {
sidebar: {{ page.sidebar | json_encode }},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change the variable here, because the header.swig will be cached when using hexo g.

@tsanie
Copy link
Contributor Author

tsanie commented Jan 28, 2018

Now here is the demo. (my theme config sidebar.display is 'post')

@sli1989
Copy link
Collaborator

sli1989 commented Jan 29, 2018

it means toc control sidebar originally?
how about use front-matter to control sidebar in pages?

@tsanie
Copy link
Contributor Author

tsanie commented Jan 29, 2018

@sli1989 Originally it has 2 conditions to control the sidebar, whether it has TOC and NexT config sidebar.display.

Now the PR is introducing a new condition, which defined in the front-matter page by page.

@sli1989
Copy link
Collaborator

sli1989 commented Jan 29, 2018

@tsanie when sidebar.display=post and there are few words in the post content, the sidebar still note display.
i mean is there another way just use the attribute of front matter instead of add a new variable, which is more convenience for posted pages.


it's also useful for new custom page using sidebar: true, maybe use AND conditional .

@tsanie
Copy link
Contributor Author

tsanie commented Jan 29, 2018

@sli1989 yeah, I think this PR could satisfy you. Just add the attribute like sidebar: true in the front matter of the post, the sidebar will force to show (even it has no TOC)

{# Exports some front-matter variables to Front-End #}
<script type="text/javascript" id="page.configurations">
CONFIG.page = {
sidebar: {{ page.sidebar | json_encode }},
Copy link
Collaborator

@sli1989 sli1989 Jan 29, 2018

Choose a reason for hiding this comment

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

how about sidebar: {{ page.title | page.sidebar | json_encode }}, ?
make sure the sidebar diaplay in post with titile when CONFIG.sidebar.display === 'post' || CONFIG.sidebar.display === 'always', don't need to add sidebar: true in front matter of posted pages where have few words (no toc situation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will make a different change with the original logic. The original logic is the sidebar.display === 'post' || sidebar.display === 'always' AND the page has a TOC in the meanwhile.

Now the code is only add an additional option that allow the user to change it. I think it is better without a breaking change. 😃

@tsanie tsanie merged commit fb373d6 into theme-next:master Jan 29, 2018
@tsanie tsanie added this to the v6.0.3 milestone Jan 29, 2018
@tsanie tsanie deleted the sidebar-settings-in-post branch January 29, 2018 08:33
@HaoweiCh
Copy link

hello bro, I just modified 1 place that can support this feature.. I didn't verify other schemes I use latest next & mist.

hope this can help you

// source/js/src/utils.js & line 228

   displaySidebar: function() {
-    if (!this.isDesktop() || this.isPisces() || this.isGemini()) {
+    if (!this.isDesktop() || this.isPisces() || this.isGemini() || CONFIG.page.sidebar===false) {
       return;
     }

I didn't know whether other place has to me modified , to my thought

tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
Support to set whether the sidebar would be shown in each single post.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants