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

Remember last opened side panel in the editor on page load #9269

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

laymonage
Copy link
Member

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?2
    • Please list the exact browser and operating system versions you tested:
      Chrome 105.0.5195.125, Firefox 105.0.1, Safari 16.0
    • Please list which assistive technologies 3 you tested:

Please describe additional details for testing this change.

  • Open the page/snippet editor
  • Open one of the side panels
  • Reload the page
  • The last opened side panel should remain open on initial page load
  • Close the side panel
  • Reload the page
  • The side panel should remain closed on initial page load
  • Go to the page explorer
  • Ensure that the behaviour of the side panel on the page explorer is completely independent of the page editor. It shouldn't remember the last opened side panel (i.e. it's always closed on initial page load) and opening/closing the side panel in the explorer shouldn't affect the side panel preferences in the page editor.

Footnotes

  1. Development Testing

  2. Browser and device support

  3. Accessibility Target

@squash-labs
Copy link

squash-labs bot commented Oct 3, 2022

Manage this branch in Squash

Test this branch here: https://laymonageremember-side-panel-p-tf392.squash.io

@zerolab
Copy link
Contributor

zerolab commented Oct 3, 2022

@laymonage would we worth tackling #9216 (comment) while at it. That is, have the info panel open by default (if no previous "last opened side panel" state exists). @thibaudcolas what do you think?

@laymonage
Copy link
Member Author

@zerolab Not sure if that would be a good idea as that means the info panel would cover the entire screen on first load on smaller screens. We could add a check to only do this on a certain minimum device screen width, but I personally would rather avoid having such checks in JS.

Besides, it's only one click away for users to show the info panel and it stays there on page reload. I get the point that it'd be useful to have it open by default for users who are new to 4.0, though...

@lb-
Copy link
Member

lb- commented Oct 3, 2022

A few thoughts

  • Are we assuming too much for the user with this feature? Sometimes being too smart can cause confusion for the user. Refreshing the page intentionally usually means I want to reset the page state. Not against this but just trying to understand.
  • Why are we using local storage and not session storage? Won't local storage keep this setting for a long time and also across other pages. I would assume that if you open a different edit page in a different tab you don't want the panel opened?
  • When we use Local or Session storage we should not assume the device has given us permission - wrap it in a try / catch is usually a good practice (I know we may not do this elsewhere)
  • Is a URL query Param a better solution here? It will be able to be added without adding to the browser history easily and also will be 'kept' on duplicate tab, on top of this the URL can be read sever side so we can set intiial state / classes in the DOM without any additional JS complexity. URL Params also means we can deep link into a page with the relevant panel open and users can also share those links.

@laymonage laymonage mentioned this pull request Oct 4, 2022
6 tasks
@thibaudcolas
Copy link
Member

@lb- the idea here is that there isn’t much of a point in not having a side panel opened. Our initial designs for this were to always have the "info" panel opened by default, but as discussed briefly above, it feels better to just keep opened whatever the user had opened last.

The idea is definitely that this would stay throughout multiple editing sessions. I think it’d be even more confusing if we did this per page or per session. For that reason as well I don’t think a URL param is the right approach, we want this for all editing that involves side panels. We could use a cookie like we do for the collapsed sidebar, but I don’t think it’s as worthwhile as there is no layout shift involved in displaying a panel.

@lb-
Copy link
Member

lb- commented Oct 7, 2022

@thibaudcolas thanks for explaining - no further push back from me on this.

For session storage - we should wrap usage in a try / catch - for a small bit of progressive enhancement if the user has disabled some things in their browser.

Good point about the layout shift.

Thanks.

And thanks @laymonage for working on this.

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

All working great @laymonage. This is missing the error handing for localStorage, but in this instance for all scenarios the error handling is just "proceed" as if nothing was saved – so this feels small enough for me to add now as part of merging.

@@ -43,6 +43,7 @@ function initPreview() {
const deviceWidth = event.target.dataset.deviceWidth;

setPreviewWidth(deviceWidth);
localStorage.setItem('wagtail-preview-panel-device', device);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
localStorage.setItem('wagtail-preview-panel-device', device);
localStorage.setItem('wagtail:preview-panel-device', device);

For consistency with e.g. the sprite data.

Also needs to be wrapped in a try as localStorage retrieval will crash hard in private browsing.

setPreviewWidth();

// Remember last selected device size
const lastDevice = localStorage.getItem('wagtail-preview-panel-device');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const lastDevice = localStorage.getItem('wagtail-preview-panel-device');
const lastDevice = localStorage.getItem('wagtail:preview-panel-device');

@thibaudcolas thibaudcolas merged commit 01dbaba into wagtail:main Oct 14, 2022
Yekasumah pushed a commit to Yekasumah/wagtail- that referenced this pull request Nov 1, 2022
)

Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
@laymonage laymonage mentioned this pull request Jul 25, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Contents of the status panel available at a glance
4 participants