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

Remove getLayout for reset-password.tsxs #4017

Closed
wants to merge 5 commits into from

Conversation

tordans
Copy link
Contributor

@tordans tordans commented Dec 23, 2022

What are the changes and their implications?

The other three auth templates have the <Layout> right in the component. This brings the reset-password file(s) in line. It also gets rid of the empty wrapper-<div>.

Feature Checklist

I had trouble getting all tests running due to ELIFECYCLE errors. In case this is broken, feel free to just close the ticket.

The other three auth templates have the `<Layout>` right in the component. This bringt the `reset-password` file(s) in line. It also gets rid of the empty wrapper-`<div>`.
@changeset-bot
Copy link

changeset-bot bot commented Dec 23, 2022

🦋 Changeset detected

Latest commit: b5afe5f

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

@dillondotzip
Copy link
Member

Hey @tordans getLayout was giving you the ELIFECYCLE error?

@tordans
Copy link
Contributor Author

tordans commented Dec 29, 2022

Hey @tordans getLayout was giving you the ELIFECYCLE error?

I don't think so. Those are my first commits to blitz so I never had a setup than worked. Did not look too much into and hoped the CI would show if there really is an error.

This PR is just a cleanup to get the generated files in sync (this one is the only one using getLayout; and AFAIK it is not needed here).

@flybayer flybayer self-requested a review as a code owner January 12, 2023 00:18
Comment on lines +2 to +3
"toolkit-app": patch
"toolkit-app-passport": patch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"toolkit-app": patch
"toolkit-app-passport": patch

@flybayer
Copy link
Member

flybayer commented Feb 7, 2023

Hey sorry @tordans but it's better to use getLayout to prevent unmounting. It would be better to change the other uses of <Layout>

@vitaliemiron
Copy link
Contributor

Hey sorry @tordans but it's better to use getLayout to prevent unmounting. It would be better to change the other uses of <Layout>

How this prevents unmounting?

Could you provide more information please? I feel the docs are not enough to understand the problem.

@flybayer
Copy link
Member

@vitaliemiron this explains it pretty well: https://adamwathan.me/2019/10/17/persistent-layout-patterns-in-nextjs/

@flybayer
Copy link
Member

flybayer commented Oct 3, 2023

closing in favor of #4146

@flybayer flybayer closed this Oct 3, 2023
@blitzjs-bot blitzjs-bot added this to Done in Toolkit Oct 3, 2023
@tordans tordans deleted the remove-getLayout branch October 4, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants