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

fix(v2): use regular div instead of main tag for wrapper layout page #1978

Merged
merged 1 commit into from
Nov 13, 2019

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 12, 2019

Motivation

Now I just noticed that the main tag is already used in the template, so it makes sense to replace it with a regular div.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See HTML markup, no changes UI.

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Nov 12, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 12, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 0fa575e

https://deploy-preview-1978--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 0fa575e

https://deploy-preview-1978--docusaurus-preview.netlify.com

@yangshun
Copy link
Contributor

yangshun commented Nov 13, 2019

Maybe we should change doc to not use <main> instead?

@endiliey
Copy link
Contributor

Maybe we should change doc to not use

instead?

The <main> tag specifies the main content of a document.
The content inside the <main> element should be unique to the document. It should not contain any content that is repeated across documents such as sidebars, navigation links, copyright information, site logos, and search forms according to

By putting main on wrapper layout, we're not letting user to fine-grain control what's the main content for pages. iirc I put <main> in doc because algolia and search bot crawls our content better, not crawling unnecessary stuff

<Layout>
<div className={styles.docPage}>
{sidebar && (
<div className={styles.docSidebarContainer}>
<DocSidebar
docsSidebars={docsSidebars}
location={location}
sidebar={sidebar}
sidebarCollapsible={sidebarCollapsible}
/>
</div>
)}
<main className={styles.docMainContainer}>
<MDXProvider components={MDXComponents}>
{renderRoutes(route.routes)}
</MDXProvider>
</main>
</div>
</Layout>

This is correct fix, we must not have two <main>

@lex111
Copy link
Contributor Author

lex111 commented Nov 13, 2019

No, main is used also on pages, it seems to me that I need to let the user determine where to use this tag. Especially main now covers sidebar, rather just text content.

@endiliey
Copy link
Contributor

@lex111 we had the same thought 😆

@yangshun yangshun merged commit 137e548 into facebook:master Nov 13, 2019
@yangshun
Copy link
Contributor

Cool, thanks for explaining!

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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants