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

fix(document-driven): rendering flash #1336

Merged
merged 14 commits into from
Jul 7, 2022
Merged

fix(document-driven): rendering flash #1336

merged 14 commits into from
Jul 7, 2022

Conversation

farnabaz
Copy link
Member

@farnabaz farnabaz commented Jul 6, 2022

🔗 Linked issue

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

  • Embed NuxtLayout inside the document-driven page
  • Overwrite the main component and remove wrapper NuxtLayout

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@nuxt-studio-dev
Copy link

Nuxt preview for Nuxt Content Docs: http://localhost:3000?_preview=b0f45de989d663ee5dcca6932fcb84477b10f8136152757fad1bc6b79e17f209

@nuxt-studio-dev
Copy link

Nuxt preview for Nuxt Content Playground: http://localhost:3000?_preview=841e245f9cde6a9a0e5729fe3235852d6b369eae78076ca4d392b8e5af0baeb5

@netlify
Copy link

netlify bot commented Jul 6, 2022 •
edited
Loading

✅ Deploy Preview for nuxt-content ready!

Name Link
🔨 Latest commit 06ab94e
🔍 Latest deploy log https://app.netlify.com/sites/nuxt-content/deploys/62c58bfa167530000837e31e
😎 Deploy Preview https://deploy-preview-1336--nuxt-content.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

atinux
atinux reviewed Jul 6, 2022
View reviewed changes
src/module.ts
nuxt.hook('app:resolve', (app) => {
if (app.mainComponent?.includes('@nuxt/ui-templates')) {
app.mainComponent = resolveRuntimeModule('./app.vue')
}
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of reading the mainComponent when we cannot overwrite it, check if it includes (<NuxtLayout) and warn the user to use <NuxtLayout> in the page to avoid layout shift with the document driven mode?

@atinux
Copy link
Member

atinux commented Jul 6, 2022

Seems we have an issue with HMR when changing the layout prop:

Screenshot.2022-07-06.at.14.03.28.mp4

@atinux
Copy link
Member

atinux commented Jul 6, 2022 •
edited
Loading

Also seems that the layout is not applied for http://localhost:3000/vue-file (yarn dev document-driven)

EDIT: updated to use <NuxtLayout> in the vue-file.vue page

@farnabaz
Copy link
Member Author

farnabaz commented Jul 6, 2022

Added watch in DD page to handle HMR e63306c

@Tahul
Copy link
Contributor

Tahul commented Jul 6, 2022

Before merging this could we please link this PR to the issue upstream related to that rendering issue?

Also could we leave TODO's in code pointing to this PR or the tracked issue.

Just want to be sure this is not considered as long term workaround but more as an hot fix we consider acceptable for a minor release, that will transparently be replaced by the long term solution when the upstream issue gets resolved.

@atinux Volta.net
Copy link
Member

atinux commented Jul 6, 2022

Agree, using <NuxtLayout> in pages will remount the layout on page changes, so not exactly the long term goal we want.

@farnabaz
Copy link
Member Author

farnabaz commented Jul 6, 2022 •
edited
Loading

The issue is, when this resolved in upstream, it will be breaking change for users that have custom app.vue in their projects

@Tahul
Copy link
Contributor

Tahul commented Jul 7, 2022

Here is a (incomplete) list of issues to track that will most likely improve the situation:

These issues have kindly been forwarded by @danielroe, thanks a lot for this!

These are not directly causing this issue looks more like a combination of factors that could have impact on what is wrong for us.

Considering this PR, I think we can move forward with it and prepare the migration notes for users when they will have to rollback the workarounds we are forced to use to (partially) fix this rendering flash.

@atinux atinux merged commit 230bbad into main Jul 7, 2022
@atinux atinux deleted the fix/dd-layout-shift branch July 7, 2022 14:48
@farnabaz farnabaz mentioned this pull request Aug 12, 2022
Merged
farnabaz added a commit that referenced this pull request Sep 7, 2022
@farnabaz @danielroe
@atinux @Tahul
fix(document-driven): rendering flash (#1336)
ebf3e84 
Co-authored-by: Daniel Roe <[email protected]>
Co-authored-by: Sébastien Chopin <[email protected]>
Co-authored-by: Yaël Guilloux <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atinux atinux atinux 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.
4 participants
@farnabaz @atinux @Tahul @danielroe