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

Mobile: Enhance the redux mobile enhancer to use the MediaQueries API #4164

Merged
merged 1 commit into from Dec 28, 2017

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Dec 26, 2017

For more performance

This PR updates the Redux Mobile Enhancer to use window.matchMedia which matches redux-responsive behavior. As it's more performant than relying on the browser's onResize event.

Testing instructions

  • Check that the mobile sidebar state is not tied to the desktop sidebar state (resize and toggle the sidebar multiple times)

@youknowriad youknowriad self-assigned this Dec 26, 2017

@youknowriad youknowriad requested a review from aduth Dec 26, 2017

@jorgefilipecosta

Changes look good to me. I did a set of tests and no regression was found when compared to master 👍
I found a regression on master thought, sidebar on mobile keeps open on refresh if it was open before, but it's not related to this changes so feel free to merge.

export function browser( state = {}, action ) {
if ( action.type === 'BROWSER_RESIZE' ) {
return { width: action.width, height: action.height };
export function mobile( state = false, action ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Dec 28, 2017

Member

minor: I would prefer if the state shape was something like state.browser.breakpoints.mobile or state.breakpoints.mobile, as it better identifies the meaning the values when seeing the state directly and leaves the door open for the addition of new breakpoints.

@jorgefilipecosta

jorgefilipecosta Dec 28, 2017

Member

minor: I would prefer if the state shape was something like state.browser.breakpoints.mobile or state.breakpoints.mobile, as it better identifies the meaning the values when seeing the state directly and leaves the door open for the addition of new breakpoints.

This comment has been minimized.

@youknowriad

youknowriad Dec 28, 2017

Contributor

I personally like the simplicity of the boolean state value. Let's keep it as is and revisit if we need more breakpoints.

@youknowriad

youknowriad Dec 28, 2017

Contributor

I personally like the simplicity of the boolean state value. Let's keep it as is and revisit if we need more breakpoints.

@youknowriad youknowriad merged commit edf17df into master Dec 28, 2017

3 checks passed

codecov/project 39.09% (+<.01%) compared to ecee09e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/redux-mobile-enhancer branch Dec 28, 2017

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