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

Chrome: Persist the state of the sidebar across page refresh #2462

Merged
merged 7 commits into from Aug 22, 2017

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Aug 18, 2017

This PR bootstrap the work to persist some UI preferences locally. (It uses localStorage)
For now, this persists only the sidebar state (opened/closed) but could be updated later to include the state of the difference sidebar panels.

solves #450 partially

@youknowriad youknowriad added the Chrome label Aug 18, 2017

@youknowriad youknowriad self-assigned this Aug 18, 2017

@youknowriad youknowriad requested review from mtias, jasmussen and aduth Aug 18, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 18, 2017

Codecov Report

Merging #2462 into master will increase coverage by 0.39%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2462      +/-   ##
==========================================
+ Coverage   26.98%   27.37%   +0.39%     
==========================================
  Files         159      161       +2     
  Lines        4903     4968      +65     
  Branches      817      830      +13     
==========================================
+ Hits         1323     1360      +37     
- Misses       3035     3055      +20     
- Partials      545      553       +8
Impacted Files Coverage Δ
editor/index.js 0% <ø> (ø) ⬆️
editor/store-persist.js 100% <100%> (ø)
editor/selectors.js 96.32% <100%> (+0.08%) ⬆️
editor/reducer.js 88.73% <100%> (ø)
editor/store.js 83.33% <83.33%> (ø)
blocks/inspector-controls/range-control/index.js 83.33% <0%> (-16.67%) ⬇️
blocks/library/paragraph/index.js 38.46% <0%> (-8.6%) ⬇️
blocks/editable/tinymce.js 0% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccf991a...c932c96. Read the comment docs.

codecov bot commented Aug 18, 2017

Codecov Report

Merging #2462 into master will increase coverage by 0.39%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2462      +/-   ##
==========================================
+ Coverage   26.98%   27.37%   +0.39%     
==========================================
  Files         159      161       +2     
  Lines        4903     4968      +65     
  Branches      817      830      +13     
==========================================
+ Hits         1323     1360      +37     
- Misses       3035     3055      +20     
- Partials      545      553       +8
Impacted Files Coverage Δ
editor/index.js 0% <ø> (ø) ⬆️
editor/store-persist.js 100% <100%> (ø)
editor/selectors.js 96.32% <100%> (+0.08%) ⬆️
editor/reducer.js 88.73% <100%> (ø)
editor/store.js 83.33% <83.33%> (ø)
blocks/inspector-controls/range-control/index.js 83.33% <0%> (-16.67%) ⬇️
blocks/library/paragraph/index.js 38.46% <0%> (-8.6%) ⬇️
blocks/editable/tinymce.js 0% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccf991a...c932c96. Read the comment docs.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 18, 2017

Contributor

Hoooraaay! It's like my birthday! I love love this, and it works great!

Contributor

jasmussen commented Aug 18, 2017

Hoooraaay! It's like my birthday! I love love this, and it works great!

Show outdated Hide outdated editor/selectors.js Outdated
@@ -0,0 +1,64 @@
/**

This comment has been minimized.

@aduth

aduth Aug 18, 2017

Member

Thanks for splitting off this file. I've been wanting to separate reducers from store initialization for a while 👍

@aduth

aduth Aug 18, 2017

Member

Thanks for splitting off this file. I've been wanting to separate reducers from store initialization for a while 👍

Show outdated Hide outdated editor/store.js Outdated
Show outdated Hide outdated editor/test/reducer.js Outdated
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 18, 2017

Member

Should userData reducer be collapsed into preferences as well, so we can remember recent blocks?

Member

aduth commented Aug 18, 2017

Should userData reducer be collapsed into preferences as well, so we can remember recent blocks?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 18, 2017

Member

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Member

aduth commented Aug 18, 2017

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 21, 2017

Contributor

Should userData reducer be collapsed into preferences as well, so we can remember recent blocks?

Let's keep this PR as a bootstrap PR for preferences persistence and leave this for later. Also thinking about using this to store the open/closed status of the sidebar panels.

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Yes probably but AFAIK this endpoint does not exist yet (see #1455). Local storage is an acceptable v1 for me.

Contributor

youknowriad commented Aug 21, 2017

Should userData reducer be collapsed into preferences as well, so we can remember recent blocks?

Let's keep this PR as a bootstrap PR for preferences persistence and leave this for later. Also thinking about using this to store the open/closed status of the sidebar panels.

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Yes probably but AFAIK this endpoint does not exist yet (see #1455). Local storage is an acceptable v1 for me.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 21, 2017

Member

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Yes probably but AFAIK this endpoint does not exist yet (see #1455). Local storage is an acceptable v1 for me.

Sorry, I should have avoided the word API as it's overused 😄 I meant the window.setUserSetting method which tracks to localStorage intended for preferences like these. I believe it also eventually persists to the database to bootstrap after being cleared. Also probably manages the case where user logs out and another logs in (those settings should be cleared, or switched to the new user).

It was previously used in the stats tracking code: https://github.com/WordPress/gutenberg/pull/2140/files#diff-17b4c77ddf8f9543bfb9ff3e64b0a6dbR23

Member

aduth commented Aug 21, 2017

Should we have used existing user settings APIs instead of localStorage to save/retrieve preferences?

Yes probably but AFAIK this endpoint does not exist yet (see #1455). Local storage is an acceptable v1 for me.

Sorry, I should have avoided the word API as it's overused 😄 I meant the window.setUserSetting method which tracks to localStorage intended for preferences like these. I believe it also eventually persists to the database to bootstrap after being cleared. Also probably manages the case where user logs out and another logs in (those settings should be cleared, or switched to the new user).

It was previously used in the stats tracking code: https://github.com/WordPress/gutenberg/pull/2140/files#diff-17b4c77ddf8f9543bfb9ff3e64b0a6dbR23

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 21, 2017

Contributor

Sorry, I should have avoided the word API as it's overused

:) I was not aware of these functions. Should we declare a script dependency or we just assume these functions are available for WP code?

Contributor

youknowriad commented Aug 21, 2017

Sorry, I should have avoided the word API as it's overused

:) I was not aware of these functions. Should we declare a script dependency or we just assume these functions are available for WP code?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 21, 2017

Contributor

@aduth It looks like we can't use this API to store complex JSON object. It's for scalar values only. If I use JSON.stringify/JSON.parse, it looks like the string is sanitized or something.

Contributor

youknowriad commented Aug 21, 2017

@aduth It looks like we can't use this API to store complex JSON object. It's for scalar values only. If I use JSON.stringify/JSON.parse, it looks like the string is sanitized or something.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 21, 2017

Member

@aduth It looks like we can't use this API to store complex JSON object. It's for scalar values only. If I use JSON.stringify/JSON.parse, it looks like the string is sanitized or something.

Yeah, digging into it a bit more, I don't know that the setUserSettings method is appropriate for this at the moment. Specifically, the current implementation works off of cookies, which don't allow for much storage:

https://github.com/WordPress/wordpress-develop/blob/7157569/src/wp-includes/js/utils.js#L151-L184

I'm wondering if it might be worth pursuing (separately) some improvements to this function to use either localStorage or indexedDB, and to accept more than simple scalar values for storage.

In the meantime, I think it would be appropriate to stick to localStorage, though I'm still concerned about the "logout" case. Do you have any ideas for how we could address this, here or separately?

Member

aduth commented Aug 21, 2017

@aduth It looks like we can't use this API to store complex JSON object. It's for scalar values only. If I use JSON.stringify/JSON.parse, it looks like the string is sanitized or something.

Yeah, digging into it a bit more, I don't know that the setUserSettings method is appropriate for this at the moment. Specifically, the current implementation works off of cookies, which don't allow for much storage:

https://github.com/WordPress/wordpress-develop/blob/7157569/src/wp-includes/js/utils.js#L151-L184

I'm wondering if it might be worth pursuing (separately) some improvements to this function to use either localStorage or indexedDB, and to accept more than simple scalar values for storage.

In the meantime, I think it would be appropriate to stick to localStorage, though I'm still concerned about the "logout" case. Do you have any ideas for how we could address this, here or separately?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 21, 2017

Contributor

In the meantime, I think it would be appropriate to stick to localStorage, though I'm still concerned about the "logout" case. Do you have any ideas for how we could address this, here or separately?

An option would be to add a "logout" hook and clear the localStorage key, the only concern is the duplication of the storage key constant in PHP.

Contributor

youknowriad commented Aug 21, 2017

In the meantime, I think it would be appropriate to stick to localStorage, though I'm still concerned about the "logout" case. Do you have any ideas for how we could address this, here or separately?

An option would be to add a "logout" hook and clear the localStorage key, the only concern is the duplication of the storage key constant in PHP.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 22, 2017

Contributor

@aduth It looks like the User Settings API rely on a global userSettings.uid provided by WP to ensure uniqueness. So I pushed a simple fix appending this global to the storage key.

Contributor

youknowriad commented Aug 22, 2017

@aduth It looks like the User Settings API rely on a global userSettings.uid provided by WP to ensure uniqueness. So I pushed a simple fix appending this global to the storage key.

aduth added some commits Aug 22, 2017

@aduth

aduth approved these changes Aug 22, 2017

Looks good 👍

const newStateValue = store.getState()[ reducerKey ];
if ( newStateValue !== currentStateValue ) {
currentStateValue = newStateValue;
window.localStorage.setItem( storageKey, JSON.stringify( currentStateValue ) );

This comment has been minimized.

@aduth

aduth Aug 22, 2017

Member

Currently this is probably fine since we're not expecting the specific state value to change very often, but this might be something worth debouncing or throttling in the future.

@aduth

aduth Aug 22, 2017

Member

Currently this is probably fine since we're not expecting the specific state value to change very often, but this might be something worth debouncing or throttling in the future.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 22, 2017

Contributor

🎉💓💓💓🤘

Contributor

jasmussen commented Aug 22, 2017

🎉💓💓💓🤘

@youknowriad youknowriad merged commit 3c6bd72 into master Aug 22, 2017

3 checks passed

codecov/project 27.37% (+0.39%) compared to ccf991a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the add/persist-preferences branch Aug 28, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 28, 2017

Member

Should mobile be exempt from this persistence? Noting while switching to emulation that the sidebar being opened sticks across page loads for mobile, which while unlikely to occur in real-world usage seems odd (i.e. open sidebar on mobile, refresh page, would expect sidebar to be closed).

Member

aduth commented Aug 28, 2017

Should mobile be exempt from this persistence? Noting while switching to emulation that the sidebar being opened sticks across page loads for mobile, which while unlikely to occur in real-world usage seems odd (i.e. open sidebar on mobile, refresh page, would expect sidebar to be closed).

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2017

Contributor

(i.e. open sidebar on mobile, refresh page, would expect sidebar to be closed)

I agree with 👆

Contributor

jasmussen commented Sep 25, 2017

(i.e. open sidebar on mobile, refresh page, would expect sidebar to be closed)

I agree with 👆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment