silverstripe / silverstripe-admin

Silverstripe Admin Component
BSD 3-Clause "New" or "Revised" License
25 stars 91 forks source link

FIX Remove current active tab from session storage after use #1683

Closed satrun77 closed 4 months ago

satrun77 commented 4 months ago

Description

The PR fixes the code that attempts to remember the tab state in the page edit screen. Currently, the tab state is stored in browser session storage but never removed, so the next time a CMS author edits the same page, it loads another tab instead of the main tab.

The fix is to remove the tab state after navigating away from the page. Note that the state is not removed if the URL from the address bar is changed manually.

https://github.com/silverstripe/silverstripe-admin/assets/166450/44141843-cc67-498d-b254-e982f9cf5a18

Manual testing steps

Issues

Pull request checklist

Parent issue

michalkleiner commented 4 months ago

The change looks ok to me, haven't tested it.

For the situation when the URL is changed manually, could we add a TTL to the saved value and if it's expired, reset the saved tab to the default? Could be quite short lived I'd say.

Alternatively, could it be safe to assume there should only be one tab stored for the current session, so any other URL can remove the saved state for other pages? Do we need to index the storage by the URL at all?

GuySartorelli commented 4 months ago

In your issue you've indicated that this issue affects "basically every version ever made" - which includes CMS 4. Can you please retarget this PR to the 1.13 branch so we can fix it there at the same time?

You'll need to reset your commits and rebuild the js after retargetting it.

satrun77 commented 4 months ago

In your issue you've indicated that this issue affects "basically every version ever made" - which includes CMS 4. Can you please retarget this PR to the 1.13 branch so we can fix it there at the same time?

You'll need to reset your commits and rebuild the js after retargetting it.

I copied Max's issue 🤣 but the code is a little different there

GuySartorelli commented 4 months ago

I copied Max's issue 🤣 but the code is a little different there

Nevertheless, if it is a bug in CMS 4 (which should be pretty quick to verify), we should be fixing it there.

GuySartorelli commented 4 months ago

Can you please force push so CI reruns correctly? It's stuck thinking you're trying to push to 2.1 because you pushed changes before swapping the target branch 😅

GuySartorelli commented 4 months ago

Looks good and says what it does on the tin. Have you put any thought into Michal's comment about ways we could have this work for accessing the page by URL?

satrun77 commented 4 months ago

Looks good and says what it does on the tin. Have you put any thought into Michal's comment about ways we could have this work for accessing the page by URL?

Yes, It all depends on what the almighty James wants. if he wants me to fix that, I will work on it.

GuySartorelli commented 4 months ago

Fair enough, please let us know whether you'll work on that or not. If you don't, I'd still be inclined to merge this since it is a clear improvement on the previous behaviour. But I'll wait to know if you'll continue working on it or not.

satrun77 commented 4 months ago

Fair enough, please let us know whether you'll work on that or not. If you don't, I'd still be inclined to merge this since it is a clear improvement on the previous behaviour. But I'll wait to know if you'll continue working on it or not.

He is happy with the current state of the change, this is all I can do now. Thanks