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

Don't load isSidebarOpened preference from localStorage on mobile. #3331

Merged
merged 2 commits into from Nov 17, 2017

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Nov 3, 2017

Description

This PR aims to close #2816. When loading preferences from the localStorage if we are on mobile it sets isSidebarOpened to false instead of using what is stored. The technique used to define if we are on mobile is the same that is already used to set the default value for the isSidebarOpened when not loading from localStorage.

Testing

Resize your window on to mobile sizes (or test on mobile). Open sidebar if it is not already open. Refresh the page and see sidebar is now closed.
Verify that other preferences are still persisted e.g visual/text mode.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 3, 2017

Codecov Report

Merging #3331 into master will increase coverage by 0.08%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3331      +/-   ##
==========================================
+ Coverage   34.85%   34.93%   +0.08%     
==========================================
  Files         261      263       +2     
  Lines        6717     6730      +13     
  Branches     1224     1228       +4     
==========================================
+ Hits         2341     2351      +10     
- Misses       3692     3694       +2     
- Partials      684      685       +1
Impacted Files Coverage Δ
editor/store.js 83.33% <ø> (ø) ⬆️
editor/constants.js 100% <100%> (ø)
editor/utils/mobile/index.js 57.14% <57.14%> (ø)

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 804ab1e...473599d. Read the comment docs.

codecov bot commented Nov 3, 2017

Codecov Report

Merging #3331 into master will increase coverage by 0.08%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3331      +/-   ##
==========================================
+ Coverage   34.85%   34.93%   +0.08%     
==========================================
  Files         261      263       +2     
  Lines        6717     6730      +13     
  Branches     1224     1228       +4     
==========================================
+ Hits         2341     2351      +10     
- Misses       3692     3694       +2     
- Partials      684      685       +1
Impacted Files Coverage Δ
editor/store.js 83.33% <ø> (ø) ⬆️
editor/constants.js 100% <100%> (ø)
editor/utils/mobile/index.js 57.14% <57.14%> (ø)

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 804ab1e...473599d. Read the comment docs.

@aduth

The storePersist utility was created to be a generic solution for store persistence. The changes here introduce editor-specific awareness into rehydration logic, against this goal.

I could see this being:

  • An editor-specific middleware monitoring REDUX_REHYDRATE and overriding the isSidebarOpened property?
  • A condition within the reducer itself? Maybe too much environment awareness for a reducer.
  • Callback on the storePersist utility to manipulate the restored value before it is rehydrated?
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 3, 2017

Member

Hi @aduth, thank you for your review and sharing the options. I end up refactoring the code to use a callback on storePersist.

Member

jorgefilipecosta commented Nov 3, 2017

Hi @aduth, thank you for your review and sharing the options. I end up refactoring the code to use a callback on storePersist.

Show outdated Hide outdated editor/store-persist.js
Show outdated Hide outdated editor/store-persist.js
Show outdated Hide outdated editor/utils/mobile.js
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 8, 2017

Member

It seems like you can pass the result of execution wp_is_mobile together with other settings when creating an editor instance and Redux state. Here is the place in the code where it could be included:

$editor_settings = array(
. I have no idea if that is the correct approach in this context, but it would simplify things and ensure there is only one way of setting mobile view.

Member

gziolo commented Nov 8, 2017

It seems like you can pass the result of execution wp_is_mobile together with other settings when creating an editor instance and Redux state. Here is the place in the code where it could be included:

$editor_settings = array(
. I have no idea if that is the correct approach in this context, but it would simplify things and ensure there is only one way of setting mobile view.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 8, 2017

Member

Hi @gziolo the problem with wp_is_mobile is that it is based on user agent and not on screen sizes. There are situations where even during the execution when a breakpoint changes (because the window resized) we want to execute some action.
To sync variables between sass and javascript I would suggest the usage of sass-variables-loader dev dependency.
I create a sample of it. Our sass variables are imported and exported in constants file so we guarantee that they are parsed just one time. In the constants file, we can also export variables that result from parsing sass variables.

Member

jorgefilipecosta commented Nov 8, 2017

Hi @gziolo the problem with wp_is_mobile is that it is based on user agent and not on screen sizes. There are situations where even during the execution when a breakpoint changes (because the window resized) we want to execute some action.
To sync variables between sass and javascript I would suggest the usage of sass-variables-loader dev dependency.
I create a sample of it. Our sass variables are imported and exported in constants file so we guarantee that they are parsed just one time. In the constants file, we can also export variables that result from parsing sass variables.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 8, 2017

Member

Similar issue applies to:

const IS_MOBILE = window && window.innerWidth < BREAK_MEDIUM;`

It's set once when the editor loads and won't change after you refresh the page. So it might not fully address the issue you are trying to solve.

Member

gziolo commented Nov 8, 2017

Similar issue applies to:

const IS_MOBILE = window && window.innerWidth < BREAK_MEDIUM;`

It's set once when the editor loads and won't change after you refresh the page. So it might not fully address the issue you are trying to solve.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 8, 2017

Member

It's set once when the editor loads and won't change after you refresh the page. So it might not fully address the issue you are trying to solve.

Yes in this case that was true because this function just needs to be applied on load when loading the prefs from localstorage. But I applied an improvement so we don't use the constant.
A sample case where we want dynamic actions on risizing is #3332. And we can mix this approaches and use this contants when setting responsive-redux breakpoints. Our sass variables become the unique place where we set window breakpoint and other places just use them.

Member

jorgefilipecosta commented Nov 8, 2017

It's set once when the editor loads and won't change after you refresh the page. So it might not fully address the issue you are trying to solve.

Yes in this case that was true because this function just needs to be applied on load when loading the prefs from localstorage. But I applied an improvement so we don't use the constant.
A sample case where we want dynamic actions on risizing is #3332. And we can mix this approaches and use this contants when setting responsive-redux breakpoints. Our sass variables become the unique place where we set window breakpoint and other places just use them.

Show outdated Hide outdated editor/store-persist.js
Show outdated Hide outdated editor/store.js
Show outdated Hide outdated editor/store.js
Show outdated Hide outdated editor/utils/mobile.js
Show outdated Hide outdated editor/utils/mobile.js
@@ -0,0 +1,10 @@
import scssVariables from '!!sass-variables-loader!./assets/stylesheets/_variables.scss';

This comment has been minimized.

@aduth

aduth Nov 9, 2017

Member

Interesting loader. Why do you need the !! prefix?

Also, for consistency we should add the Internal dependencies DocBlock.

@aduth

aduth Nov 9, 2017

Member

Interesting loader. Why do you need the !! prefix?

Also, for consistency we should add the Internal dependencies DocBlock.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 10, 2017

Member

The !! disables other configured loaders during the import process. Without this flag or import was not being successful.

@jorgefilipecosta

jorgefilipecosta Nov 10, 2017

Member

The !! disables other configured loaders during the import process. Without this flag or import was not being successful.

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 10, 2017

Member

I decided to apply the solution proposed by @gziolo and refactored the logic to be a middleware. Given that we found some possible store-persist improvements that now are not directly in the scope of this changes I separated these improvements into a different PR #3424.

Member

jorgefilipecosta commented Nov 10, 2017

I decided to apply the solution proposed by @gziolo and refactored the logic to be a middleware. Given that we found some possible store-persist improvements that now are not directly in the scope of this changes I separated these improvements into a different PR #3424.

Show outdated Hide outdated editor/store.js
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 10, 2017

Member

I haven't tested yet, but I like it as it is. Thanks for addressing my feedback 😃 I will test it on Monday if it isn't merged by that time.

Member

gziolo commented Nov 10, 2017

I haven't tested yet, but I like it as it is. Thanks for addressing my feedback 😃 I will test it on Monday if it isn't merged by that time.

@mcsf

mcsf approved these changes Nov 12, 2017

This looks quite clear and works for me. The only thing I'm not confident assessing on my own is the pertinence of the loader for scssVariables, but Jorge's explanation seems good.

Show outdated Hide outdated editor/utils/test/mobile.js
Show outdated Hide outdated editor/utils/mobile.js
@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 13, 2017

Member

Hi @mcsf thank you for your suggestions! They were applied.

Member

jorgefilipecosta commented Nov 13, 2017

Hi @mcsf thank you for your suggestions! They were applied.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 17, 2017

Member

It was approved by @mcsf, please rebase and merge. There is no need to perform another review. Those was minor updates.

Member

gziolo commented Nov 17, 2017

It was approved by @mcsf, please rebase and merge. There is no need to perform another review. Those was minor updates.

@gziolo

gziolo approved these changes Nov 17, 2017

I have already reviewed in the past and it still looks good 👍

@jorgefilipecosta jorgefilipecosta merged commit b87f59c into master Nov 17, 2017

3 checks passed

codecov/project 34.93% (+0.08%) compared to 804ab1e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/dont-load-sidebar-preference-on-mobile-collapse-by-default branch Nov 17, 2017

@@ -87,6 +87,7 @@
"react-markdown": "2.5.0",
"react-test-renderer": "16.0.0",
"sass-loader": "6.0.6",
"sass-variables-loader": "0.1.3",

This comment has been minimized.

@aduth

aduth Nov 17, 2017

Member

We should have updated package-lock.json with this dependency addition. Users running npm 5+ will encounter changes introduced when installing from a fresh clone. The recommended Node version was bumped to the latest LTS in #3294 and will include npm 5+. I suggest installing this latest version.

@aduth

aduth Nov 17, 2017

Member

We should have updated package-lock.json with this dependency addition. Users running npm 5+ will encounter changes introduced when installing from a fresh clone. The recommended Node version was bumped to the latest LTS in #3294 and will include npm 5+. I suggest installing this latest version.

This comment has been minimized.

@aduth

aduth Nov 17, 2017

Member

Pushed to master in 1709207

@aduth

aduth Nov 17, 2017

Member

Pushed to master in 1709207

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 17, 2017

Member

Thank you for decting this problem and submiting a fast fix 👍 Unfortunately I missed the update and did not detected the probelm :(

@jorgefilipecosta

jorgefilipecosta Nov 17, 2017

Member

Thank you for decting this problem and submiting a fast fix 👍 Unfortunately I missed the update and did not detected the probelm :(

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