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

Add subsite change detection #516

Closed
wants to merge 1 commit into from
Closed

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Mar 17, 2023

Closes #515

@lekoala lekoala changed the title And subsite change detection Add subsite change detection Mar 17, 2023
@GuySartorelli
Copy link
Member

This looks really useful, thanks!
As a new feature, this will need to target the 3 branch. Please retarget and rebase this PR.

@GuySartorelli GuySartorelli force-pushed the patch-3 branch 2 times, most recently from 0524bc5 to 4261f0d Compare April 18, 2023 01:01
@GuySartorelli GuySartorelli changed the base branch from 2 to 3 April 18, 2023 01:01
@GuySartorelli
Copy link
Member

@lekoala I've retargetted this at the 3 branch and made the necessary changes for it to pass the new linting rules.

However, while testing this I found a (probably very common) scenario where this will cause more confusion rather than less.
When editing a page in tab A in subsite A, after changing subsites to subsite B in tab B, tab A tells me it wants me to refresh because I changed subsites. But refreshing tab A sets the subsite back to subsite A - so it appears I haven't made any change at all - and then tab B tells me I should refresh the page because I changed subsites again!

Can you think of an easy way to resolve this in a general way? We could obviously check if someone's in a cms page edit form, but that seems like a narrow scope when this scenario could theoretically happen in other contexts as well.

@lekoala
Copy link
Contributor Author

lekoala commented Apr 18, 2023

@GuySartorelli reloading should load the proper website no? any way, it would be easy i think to pass along the SubsiteID in the url to force loading the last active subsite

@michalkleiner
Copy link
Contributor

michalkleiner commented Apr 18, 2023

The problem we had in past was that not all model admins or all CMS UI was the same for all subsites, so where should it take you on a reload when you were editing a page type that is not available for the destination subsite? Go to default pages view?

@lekoala
Copy link
Contributor Author

lekoala commented Apr 18, 2023

@michalkleiner fair point. i guess that would lead to a 404 so i'm not even sure how the redirect would work. i'm not saying my approach in 100% bulletproof (it works for me, and that's already something :) ) so maybe that's why it should be opt in?

@GuySartorelli
Copy link
Member

I'd hesitate to merge this even as an opt-in feature at the moment, since it doesn't play nicely with editing pages which is arguably the most common action in the CMS.

@michalkleiner
Copy link
Contributor

michalkleiner commented Apr 18, 2023

I would be ok to have this opt in, if it always took you to just /admin and it clearly said it in the dialog. Or even better with the message customisable, which would require adjustments to source it from config and put it e.g. in a data attribute, but that's all doable.

@GuySartorelli
Copy link
Member

@lekoala Are you still interested in having this PR merged in? If the changes Michal suggested in the last comment are made we can accept it.

@lekoala
Copy link
Contributor Author

lekoala commented Oct 6, 2023

still interested but with limited time to spend on it

@GuySartorelli
Copy link
Member

I'm going to close this for now since there has been no action for several months - we can reopen it when/if you get time to pick it up again.

@lekoala
Copy link
Contributor Author

lekoala commented Dec 14, 2023

no worries ;) it's just it's working "good enough" for me at the moment so i have limited interest in getting this further

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.

new feature: subsite change detector
3 participants