Skip to content
This repository has been archived by the owner on Jul 13, 2022. It is now read-only.

Add "Disable preview" functionality & updated packages #7

Merged
merged 4 commits into from
Aug 3, 2021

Conversation

arcataroger
Copy link
Contributor

Hi there,

Thanks for this super helpful plugin!

One of our editors let me know that the "View published page" button is currently inconsistent/confusing. If you click it after a page has been previewed, that button will still show the preview page rather than the published page. The Next.js preview mode isn't per-page but per-browser session, so subsequent pageviews launched in the same browser instance would still have preview mode active.

This PR allows an (optional) "Disable preview API route" in the settings. If specified, it goes to API route that will clear the preview mode cookies and then redirect to the slug. I updated the readme with more instructions as well.

Please note that I also had to update the packages because otherwise the plugin wouldn't build in my dev environment (node version was too new for node-sass).

Copy link
Owner

@wbobeirne wbobeirne left a comment

Choose a reason for hiding this comment

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

Ah yep this makes sense. For the site I was working on, we actually added a banner that conditionally rendered if they were in preview mode, and they could just click a button in the banner to go back to live. I would definitely recommend that to folks so they can tell when they're on previews regardless of how they got there.

Given that this is optional though, seems really low-harm to add. As for the package updates, this project was probably overdue for a yarn upgrade anyway so thanks for doing that.

Happy to merge if you can just confirm the 1 question I had.

package.json Outdated Show resolved Hide resolved
Co-authored-by: Will O'Beirne <wbobeirne@gmail.com>
@arcataroger
Copy link
Contributor Author

We had that banner too, but the editors were still confused. I don't blame them either, since most CMSes show a preview on a per-page basis rather than persisting it across the entire browser sessions and affecting ALL subsequent pageloads. That's a Next.js UX quirk and this is just a workaround.

Good catch on the default not being a placeholder! It does fill that out by default, so it makes sense to clear it.

On a related note though, on line 100 of Main.jsx, the ternary for liveHref is just disablePreviewPath ? (pathWithAPI) : (pathWithoutAPI). My JS isn't superb... I believe an empty string ("") evaluates to false, but if there's a better way to check if that parameter is filled out, please feel free to change the conditional.

Thanks again!

@arcataroger
Copy link
Contributor Author

@wbobeirne Any chance this PR could be merged? I removed the default as you suggested. Please let me know if there's anything else that needs more tweaking.

Our editors are hoping to use the new feature and I'd hate to fork the plugin just for that one thing.

Thank you!!

@wbobeirne
Copy link
Owner

Yep, absolutely. I haven't been using DatoCMS for a few months now (Was using it at $previousJob and now no longer work there) and typically I'd like to make sure it works myself which is why I was holding off, but eh I'll take your word for it.

I believe an empty string ("") evaluates to false, but if there's a better way to check if that parameter is filled out, please feel free to change the conditional.

Yeah this is all good 👌

Just FYI, you may need to uninstall and reinstall the plugin to get the update. DatoCMS doesn't have a plugin migration flow yet, so any time you add a new field it tends to not auto-update. At least that was the case a year or so ago.

@wbobeirne wbobeirne merged commit 906f7ba into wbobeirne:master Aug 3, 2021
@arcataroger
Copy link
Contributor Author

@wbobeirne I see, thank you for trusting the PR and merging the changes :) Hopefully it'll all work out.

Also, one last thing... could you please create a new release on npmjs? I updated the package.json ver to 0.2.0, but feel free to change that to something else if more appropriate.

Thank you.

@wbobeirne
Copy link
Owner

Done! Let me know how it goes for y'all.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants