Add hide_title metadata that hides the title text on the top of the doc #540

MisterTea · 2018-04-10T04:24:52Z

Motivation

Optionally hide the title from the top of the doc so people can customize the title

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Running with this on my test site

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.)

yangshun · 2018-04-10T06:49:29Z

@MisterTea Did you run prettier before submitting this PR?

JoelMarcey

Hi @MisterTea. Thanks for the PR. I have a couple of questions/suggestions.

It looks like hide_title would be part of the doc header metadata as discussed here: https://docusaurus.io/docs/en/doc-markdown.html

Can you update the docs for this new field? https://github.com/facebook/Docusaurus/blob/master/docs/api-doc-markdown.md
Run npm run prettier to make sure the code is formatted correctly.
Then I have some code comments inline...

JoelMarcey · 2018-04-10T16:14:45Z

lib/core/DocsLayout.js

@@ -23,6 +23,15 @@ class DocsLayout extends React.Component {
 if (this.props.Doc) {
 DocComponent = this.props.Doc;
 }
+ let docTitle = i18n


Someone might put

hide_title: false

in the doc header.

So, how about instead we do the converse here:

let docTitle = null; // If the attribute does not exist or is set to false, we set a title. if (!this.props.metadata.hide_title || this.props.metadata.hide_title.toLowerCase() === 'false') { docTitle = .... }

JoelMarcey · 2018-04-10T16:16:56Z

lib/core/Doc.js

@@ -60,11 +60,18 @@ class Doc extends React.Component {
 </a>
 );
 }
+
+ let title = null;
+ if (this.props.title) {


Maybe a comment here saying something like:

// this.props.title could be set to null if hide_title was set in the doc metadata

JoelMarcey · 2018-04-10T17:48:13Z

@yangshun Can you look at this too? I think it is right, but just wanted to make sure I wasn't missing anything here.

yangshun

Nice work @MisterTea ! I left some inline comments on how the code can be simplified. We should try as much not to change too much of the existing logic and it seems like it's possible for this feature to do so.

yangshun · 2018-04-11T07:11:25Z

docs/api-doc-markdown.md

@@ -13,6 +13,8 @@ Documents use the following markdown header fields that are enclosed by a line `

 `title`: The title of your document. If this field is not present, the document's `title` will default to its `id`.

+`hide_title`: Whether to hide the title at the top of each documentation page. If this is `null` or set to anything besides `false`, the title will be displayed.


It's better if this field was restricted to a boolean value for simplicity. That way, we can shorten the description to the first sentence and do less checks in our code.

@yangshun How do you control what a user might enter in the metadata for the doc though? Someone could put:

hide_title: 'blahblah'

How do we treat that?

Or are you saying that just the fact that hide_title is explicitly put in the metadata it is always truthy, no matter the associated string?

Yes I mean we will just determine the value based on whether it's truthy/falsy if a non-true/false value was provided. It'll be too much of a hassle to do type checks manually.
I believe Front matter supports boolean values (I will have to check on that though).

If a use puts 'blahblah', it will be treated as true as it is a non-empty string.

yangshun · 2018-04-11T07:14:59Z

lib/core/Doc.js

@@ -60,11 +60,17 @@ class Doc extends React.Component {
 </a>
 );
 }
+
+ let title = null;
+ // this.props.title could be set to null if hide_title was set in the doc metadata


The title value should be kept independent of the showing/hiding logic. What you can do is to pass in the prop hideTitle into this component from the parent.

You can also inline the conditional rendering logic into JSX since it's very short:

{this.props.hideTitle && <h1>{this.props.title}</h1>}

yangshun · 2018-04-11T07:17:32Z

lib/core/DocsLayout.js

@@ -23,6 +23,18 @@ class DocsLayout extends React.Component {
 if (this.props.Doc) {
 DocComponent = this.props.Doc;
 }
+ let docTitle = null;


These stuff can remain. Pass this.props.metadata.hide_title as a prop to <DocComponent>/<Doc> instead to simplify things.

yangshun · 2018-04-11T07:18:27Z

lib/core/DocsLayout.js

- this.props.metadata.title
- : this.props.metadata.title
- }
+ title={docTitle}


Pass in the hideTitle prop here - hideTitle={metadata.hide_title}

MisterTea · 2018-04-12T00:05:49Z

I think this new way is a lot cleaner, thanks. Let me know what you think!

yangshun

Thanks for the quick changes! LGTM, will let @JoelMarcey have a final look and merge

JoelMarcey

Awesome! Thanks.

JoelMarcey

I think we are missing a !, aren't we?

JoelMarcey · 2018-04-12T00:47:14Z

lib/core/Doc.js

 return (
 <div className="post">
 <header className="postHeader">
 {editLink}
- <h1>{this.props.title}</h1>
+ {this.props.hideTitle && <h1>{this.props.title}</h1>}


Wait. Shouldn't this be

!this.props.hideTitle

We only want to show this if we don't have hideTitle, right?

yangshun · 2018-04-12T00:50:37Z

Good catch! My bad for typing incorrect example code >_<

…

On Wed, Apr 11, 2018, 5:47 PM Joel Marcey ***@***.***> wrote: ***@***.**** requested changes on this pull request. I think we are missing a !, aren't we? ------------------------------ In lib/core/Doc.js <#540 (comment)>: > return ( <div className="post"> <header className="postHeader"> {editLink} - <h1>{this.props.title}</h1> + {this.props.hideTitle && <h1>{this.props.title}</h1>} Wait. Shouldn't this be !this.props.hideTitle We only want to show this if we don't have hideTitle, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#540 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQRHZwrdavxSGEiCElV951Hkcy-ee38ks5tnqQigaJpZM4TNlRI> .

JoelMarcey · 2018-04-12T00:53:28Z

I went ahead and made the update. I think we are good now.

yangshun · 2018-04-12T00:54:48Z

I'll give it a thorough test later (: On Apr 11, 2018 5:53 PM, "Joel Marcey" <notifications@github.com> wrote: *@JoelMarcey* approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#540 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQRHQSfgHWfrkwe4etqil7Z0fwwHXFHks5tnqWigaJpZM4TNlRI> .

JoelMarcey · 2018-04-12T00:57:06Z

I was going to test it locally right now ;)

JoelMarcey · 2018-04-12T01:04:22Z

Seems to work 👍

Add hide_title metadata that hides the title text on the top of the doc

5a56c62

facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 10, 2018

JoelMarcey suggested changes Apr 10, 2018

View reviewed changes

Review Feedback

388a915

JoelMarcey requested a review from yangshun April 10, 2018 17:47

yangshun added 2 commits April 11, 2018 00:06

Update api-doc-markdown.md

b346d0f

Update api-doc-markdown.md

e36f990

yangshun suggested changes Apr 11, 2018

View reviewed changes

Jason Gauci added 2 commits April 11, 2018 17:04

Review feedback

04b9870

Merge branch 'master' of github.com:MisterTea/Docusaurus

fb42b18

yangshun approved these changes Apr 12, 2018

View reviewed changes

JoelMarcey approved these changes Apr 12, 2018

View reviewed changes

JoelMarcey suggested changes Apr 12, 2018

View reviewed changes

Show title if we don't hide it.

04850b4

JoelMarcey approved these changes Apr 12, 2018

View reviewed changes

JoelMarcey merged commit 6dd6ead into facebook:master Apr 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hide_title metadata that hides the title text on the top of the doc #540

Add hide_title metadata that hides the title text on the top of the doc #540

MisterTea commented Apr 10, 2018

yangshun commented Apr 10, 2018

JoelMarcey left a comment

JoelMarcey Apr 10, 2018

JoelMarcey Apr 10, 2018

JoelMarcey commented Apr 10, 2018

yangshun left a comment

yangshun Apr 11, 2018

JoelMarcey Apr 11, 2018

yangshun Apr 11, 2018 •

edited

Loading

yangshun Apr 11, 2018

yangshun Apr 11, 2018

yangshun Apr 11, 2018

yangshun Apr 11, 2018

MisterTea commented Apr 12, 2018

yangshun left a comment

JoelMarcey left a comment

JoelMarcey left a comment

JoelMarcey Apr 12, 2018

yangshun commented Apr 12, 2018 via email

JoelMarcey commented Apr 12, 2018

yangshun commented Apr 12, 2018 via email

JoelMarcey commented Apr 12, 2018

JoelMarcey commented Apr 12, 2018

		@@ -13,6 +13,8 @@ Documents use the following markdown header fields that are enclosed by a line `

		`title`: The title of your document. If this field is not present, the document's `title` will default to its `id`.

		`hide_title`: Whether to hide the title at the top of each documentation page. If this is `null` or set to anything besides `false`, the title will be displayed.

Add hide_title metadata that hides the title text on the top of the doc #540

Add hide_title metadata that hides the title text on the top of the doc #540

Conversation

MisterTea commented Apr 10, 2018

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

yangshun commented Apr 10, 2018

JoelMarcey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelMarcey commented Apr 10, 2018

yangshun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisterTea commented Apr 12, 2018

yangshun left a comment

Choose a reason for hiding this comment

JoelMarcey left a comment

Choose a reason for hiding this comment

JoelMarcey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun commented Apr 12, 2018 via email

JoelMarcey commented Apr 12, 2018

yangshun commented Apr 12, 2018 via email

JoelMarcey commented Apr 12, 2018

JoelMarcey commented Apr 12, 2018

yangshun Apr 11, 2018 •

edited

Loading