-
Notifications
You must be signed in to change notification settings - Fork 115
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 Reload element form when element version changes #1200
FIX Reload element form when element version changes #1200
Conversation
published: PropTypes.bool, | ||
liveVersion: PropTypes.bool, | ||
isPublished: PropTypes.bool, | ||
isLiveVersion: PropTypes.bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort-of-unrelated refactoring - I noticed these were wrong when I tried to use the old keys but they kept showing undefined
30a2430
to
ee6a200
Compare
The form will be reloaded when the element is saved/published/unpublished, etc - but only if its version or versioned-state is changing.
ee6a200
to
e9776e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this method is viable unfortunately, with this PR there's now a massive FOUT that looks really bad that wan't there before. Given that LinkField's on elemental is a pretty niche use-case, and inline-publishing itself isn't broken, it's just got the incorrect label until the page is refreshed, I'd consider this new FOUT to be a regression
Also subsequent inline editing and publishing doesn't actually reload the block, so the original issue hasn't been fully fixed
I've provided a video of before and after https://www.youtube.com/watch?v=yNryWP-m0CY
I'd be inclined to just close this one as too hard because I think there needs to be a more advanced solution to resolve this.
Note that upload field has the same issue of incorreclty saying it's draft when it's published, however I don't think we've had many complaints about this so perhaps this isn't that big of a real world issue
Note to self: FOUT is "Flash Of Unstyled Text" (or in this case just flash of badness) |
A couple of potential ideas for CMS 6 which approach the problem by just not allowing inline publishing:
1's easier and 2's a stretch though if we got scrolling working it would standardise how we 'inline edit' DataObject's and we could do away with a bunch of elemental code thus reducing the maintenance surface |
Given that we've received pretty clear indications that allowing publication of individual blocks is not ideal and that we have a card for CMS 6 to remove this action, I'm inclined to agree that fixing this specific bug is not a great use of time. I think it's worth noting that it's not only LinkField that is affected by this. The UploadField has the same problem. Any relational field you might want to put in an Elemental block will have the same problem. On the FOUT point, there's a pretty straight forward fix to this problem: don't nuke the current form state right away when the form builder gets invalidated ... instead wait until you get your form schema back. That should provide a nice smooth UI transition between the two state. |
I took one look at trying that and while it is conceptually a simple idea, it is not simple to implement. I'm going to say this just isn't worth our time right now. |
Requires silverstripe/silverstripe-admin#1771 to actually fix the bug - but this PR is safe on its own as a patch as seen by CI going green.
If the prop isn't used on the other end (i.e. if someone only updates elemental and not admin) then the bug will still be there, but it won't cause any new problems.
The form will be reloaded when the element is saved/published/unpublished, etc - but only if its version or versioned-state is changing.
Issue