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.

Sign up for GitHub

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

Jump to bottom

New layout implementation #631

Merged
merged 17 commits into from
Aug 3, 2022
Merged

New layout implementation #631

merged 17 commits into from
Aug 3, 2022

Conversation

shuding
Copy link
Owner

@shuding shuding commented Aug 2, 2022

Closes #623.

The outcome of this PR:

  • No need to import the CSS file in _app
  • No need to implement the getLayout thing in _app
  • No need to have a custom _app file
  • Non-MDX pages (pages/hello.js) will not be affected by the global theme styles

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: f69c212

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 2, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextra-theme-docs-dev ✅ Ready (Inspect) Visit Preview Aug 3, 2022 at 11:46PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
nextra ⬜️ Ignored (Inspect) Aug 3, 2022 at 11:46PM (UTC)

@dimaMachina
Copy link
Collaborator

dimaMachina commented Aug 2, 2022 •
edited
Loading

How about size comparison? Imagine that our bundled style.css has 50kb and we have 100 mdx pages. Does it means that each page will have 50kb more? And total bundle will be increased by 50kb*100=5mb? 🧐

@shuding
Copy link
Owner Author

shuding commented Aug 2, 2022

How about size comparison? Imagine that our bundled style.css has 50kb and we have 100 mdx pages. Does it means that each page will have 50kb more? And total bundle will be increased by 50kb*100=5mb? 🧐

Yeah this is my biggest concern of using styled-jsx too, another choice is to use a webpack loader to magically inject that global CSS import but then it will have this (existing today) issue:

Non-MDX pages (pages/hello.js) will not be affected by the global theme styles

@shuding
Copy link
Owner Author

shuding commented Aug 2, 2022 •
edited
Loading

It would be so good if Next.js supports page-level global CSS import...

@shuding
Copy link
Owner Author

shuding commented Aug 3, 2022

It would be so good if Next.js supports page-level global CSS import...

OK I managed to make it work 😄

@shuding shuding marked this pull request as ready for review August 3, 2022 22:50
@shuding
Copy link
Owner Author

shuding commented Aug 3, 2022

Thank you for the review @B2o5T!

@shuding
Copy link
Owner Author

shuding commented Aug 3, 2022

I also have to deprecate the

<Nextra.Tabs>

use case, and built-in components won't be exposed by default. It now requires you to import import { Tabs, Tab } from 'nextra-docs-theme/Tabs' first.

This is because we are not always providing these components. So it causes issues since they might not be available (e.g. importing MDX directly).

@dimaMachina
Copy link
Collaborator

@shuding you are welcome! Amazing work

@shuding shuding merged commit c8605d6 into core Aug 3, 2022
@shuding shuding deleted the shu/cc91 branch August 3, 2022 23:53
tatukoivisto pushed a commit to tatukoivisto/nextra that referenced this pull request Aug 20, 2023
@shuding
New layout implementation (shuding#631)
f7f2147 
* refactor loader and layout resolution

* use global css import

* test ssg

* fix fast refresh

* upadte snapshot

* add changeset

* remove @edge-runtime/vm

* fix test

* fix test

* apply review suggestions

* fix string template

* update snapshot

* deprecate the Nextra.* component

* fix import

* fix import

* fix import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimaMachina dimaMachina dimaMachina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shuding @dimaMachina