-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
fix(v2): make not clickable post title on post item page #1962
Conversation
Deploy preview for docusaurus-2 ready! Built with commit c0ea177 |
Deploy preview for docusaurus-preview ready! Built with commit c0ea177 |
@@ -45,7 +51,7 @@ function BlogPostItem(props) { | |||
return ( | |||
<header> | |||
<h1 className={classnames('margin-bottom--sm', styles.blogPostTitle)}> | |||
<Link to={permalink}>{title}</Link> | |||
{isBlogPostPage ? title : <Link to={permalink}>{title}</Link>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better solution? Comparison with location.pathname
will not work, what other options are there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right solution (?). Determining whether it's a list or not using pathname parsing can lead to subtle bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is a right way to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I’m relying on this behavior 😂
This is personal preference but I think Its much more convenient for people to click on title rather than the read more.
Also, not all blog post has “Read more”. If there is no “<!—truncate” marker in the markdown, it wont have read more button
V1 also has this functionality
https://babeljs.io/blog/
https://docusaurus.io/blog/
But in posts lists, the title will also remain clickable, isn't it? This change will only affect the post item page. |
oops you are right. I misread it on mobile :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @yangshun for another approval
Is there anything else to do before it can be merged? |
I think this is OK. I find it annoying too previously (the blog list page and item page had no different so i keep on clicking the same thing). I'm not UI expert so I usually ask for @yangshun opinion on this. but LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks! Please self-merge.
Motivation
On the post page, the title should not be clickable
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)