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 Don't try to preview unlocalised objects #781

Conversation

GuySartorelli
Copy link
Contributor

@GuySartorelli GuySartorelli commented Feb 17, 2023

I don't think there's ever a scenario where you'd want to consider previews as not frontend so I don't think this is a breaking change.
I'm on the fence about whether updatePreviewLink() belongs here or in FluentExtension directly. Probably it belongs on FluentExtension so it can affect anything else which have a preview (e.g. elemental blocks which aren't inline-editable) - but I'll leave it like this for now (it is still an improvement and solves the specific exact scenario mentioned in the issue) until someone opines.

Parent issue

@GuySartorelli
Copy link
Contributor Author

Also happy to change the commit name - when I wrote it I was thinking of the last piece of the chain that I was solving, not the first part as described in the issue itself.

@tractorcow
Copy link
Collaborator

The only risk this introduced, as we identified in a prior conversation, is that some pages visible in the admin won't appear at all (i.e. 404) in preview. I still this this fix is a more consistent and better functionality, rather than showing a false view of content that then breaks after publish. :)

@GuySartorelli
Copy link
Contributor Author

@tractorcow I'm not sure if your comment is in response to something I've said, or to the changes I've introduced with this PR.... if the latter, then there is no 404 in the preview (for pages specifically) with this PR. That's what setting the preview url to null ensures. Instead, the preview panel is collapsed by default for those pages and a "there is no preview available" message is displayed if the user explicitly swaps to split or preview mode.

@GuySartorelli
Copy link
Contributor Author

@tractorcow @chrispenny do either of you have an opinion as to whether this change belongs on the FluentSiteTreeExtension or directly on FluentExtension?

@chrispenny
Copy link

@GuySartorelli Good question... Can any DataObject implement a Preview window? I don't think I've ever seen it outside of SiteTree, but I vaguely remember reading that it was possible.

If any DO can use Preview, then I guess it makes sense to be on FluentExtension.

CC: @mfendeksilverstripe

@mfendeksilverstripe
Copy link
Contributor

I think the ability to have a preview is not limited to only SiteTree. I think this belongs more to the FluentExtension. Alternatively, this can be a separate extension which is enabled by default but if needed can be disabled / overwritten via yaml config. This could give projects more flexibility over how to use this feature.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Mar 2, 2023

Can any DataObject implement a Preview window?

Yup (I even did a whole talk on it ;p). Any DataObject that implements the CMSPreviewable interface and returns some valid link path from PreviewLink() will have a preview available in gridfield edit forms and modeladmins - or in any LeftAndMain for that matter. You can even slap a preview in the siteconfig if you really want.

If any DO can use Preview, then I guess it makes sense to be on FluentExtension.
...
I think this belongs more to the FluentExtension.

I'll make the change 👍

Alternatively, this can be a separate extension which is enabled by default but if needed can be disabled / overwritten via yaml config. This could give projects more flexibility over how to use this feature.

I don't think it makes sense to configure this separately - I can't think of any use case where you'd want anything other than the correct preview being displayed in the preview panel.

@GuySartorelli GuySartorelli changed the title FIX Don't try to preview unlocalised pages FIX Don't try to preview unlocalised objects Mar 2, 2023
@GuySartorelli
Copy link
Contributor Author

Oops I broke some tests - will look at that now

Copy link
Contributor

@mfendeksilverstripe mfendeksilverstripe left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@mfendeksilverstripe mfendeksilverstripe merged commit f07b036 into tractorcow-farm:6 May 23, 2023
@GuySartorelli GuySartorelli deleted the pulls/6/no-locale-no-preview branch May 24, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants