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(v2): blog sidebar #3593

Merged
merged 4 commits into from
Oct 16, 2020
Merged

feat(v2): blog sidebar #3593

merged 4 commits into from
Oct 16, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Oct 15, 2020

Motivation

Feature parity with Docusaurus v1.

https://docusaurus.io/docs/en/adding-blog#changing-how-many-blog-posts-show-on-sidebar

Needed for RN migration, cc @Simek

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

tests + preview + dogfooding

@slorber slorber requested a review from lex111 as a code owner October 15, 2020 18:32
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 15, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 15, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 0b3a40c

https://deploy-preview-3593--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator Author

slorber commented Oct 15, 2020

@lex111 any opinion on the design we should use for this feature? (I'm not very good at design)

Here's a POC, will polish this a bit to before merge and cleanup TOC classes I reused:
https://deploy-preview-3593--docusaurus-2.netlify.app/classic/blog/2019/12/30/docusaurus-2019-recap

@lex111
Copy link
Contributor

lex111 commented Oct 15, 2020

@slorber definitely, the recent posts section should not look like TOC.

@slorber
Copy link
Collaborator Author

slorber commented Oct 16, 2020

@lex111 any design suggestion for this feature then?

Maybe you want it to be more like v1? (no border etc...)
https://prettier.io/blog/2020/08/24/2.1.0.html

I'll polish the node part but if you have a good idea don't hesitate to modify the BlogSidebar UI component, as all the design is basically there we shouldn't have any conflict

@lex111
Copy link
Contributor

lex111 commented Oct 16, 2020

Maybe you want it to be more like v1? (no border etc...)

Yes, that's what I meant. Let's remove the border, make the fonts bigger. You can look at v1 as an example https://docusaurus.io/blog/2019/12/30/docusaurus-2019-recap

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 16, 2020
@slorber
Copy link
Collaborator Author

slorber commented Oct 16, 2020

@lex111 does it look good enough to merge?
Probably not perfect but didn't want to refactor totally the blog layout either in this PR.

2 grid cols might feel a bit small compared to v1.
But v2 also has a nice UX by keeping exactly the same layout on all blog pages, which was not the case on v1 where you navigate from blogpost list to blog post and the width of the layout changes.

….module.css

Co-authored-by: Alexey Pyltsyn <lex61rus@gmail.com>
@@ -25,7 +26,10 @@ function BlogListPage(props: Props): JSX.Element {
<Layout title={title} description={blogDescription}>
<div className="container margin-vert--lg">
<div className="row">
<main className="col col--8 col--offset-2">
<div className="col col--2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of empty divs (if blogSidebarCount=0), let's handle this case properly (add col--offset-2 class if there is no sidebar).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that it requires to do this check everywhere 🤪
I'd rather encapsulate it and keep a constant layout, makes code simpler for same results.
I don't think it's a bad practice to put an empty div for layout reasons, particularly if it has a width. It's common with flexbox to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, okay, although I don't really like this approach.

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Feel free to self-merge.

@slorber slorber merged commit e4c1626 into master Oct 16, 2020
@lex111
Copy link
Contributor

lex111 commented Oct 16, 2020

Woohoo, we get 3k commits! :D

image

@slorber
Copy link
Collaborator Author

slorber commented Oct 16, 2020

Nice 😉soon 20k stars

@slorber slorber deleted the slorber/blog-sidebar branch August 17, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants