[components] Switch to mobile on resize. No need to refresh browser #807
Conversation
|
This could need a rebase against current next |
bjoerge
force-pushed
the
responsive-split-pane
branch
from
82ba468
to
014e981
Compare
kristofferjs
force-pushed
the
responsive-split-pane
branch
from
014e981
to
b5fa339
Compare
|
Rebased |
kristofferjs
force-pushed
the
responsive-split-pane
branch
from
b5fa339
to
6076118
Compare
|
rebased again. |
bjoerge
force-pushed
the
responsive-split-pane
branch
from
6076118
to
6153fc0
Compare
|
|
||
| componentWillUnmont() { | ||
| if (window) { | ||
| window.removeEventListener('resize', this.handleResize) |
There was a problem hiding this comment.
This is a performance killer as it will trigger a re-render of all split panes on every resize event. It would be much better if this event listener could've been shared between all instances and debounced.
There was a problem hiding this comment.
Does it help if i make it a passive listener?
| @@ -107,7 +132,7 @@ export default class PanesSplitController extends React.Component { | |||
|
|
|||
| // TODO We need a way to target mobile devices in JS | |||
| // --screen-medium-break: 32em; ~32 * 16 = 512 | |||
| const isMobile = window && window.innerWidth < 512 | |||
| const isMobile = this.state.windowWidth < 512 | |||
There was a problem hiding this comment.
For clarity I suggest renaming this variable to isSmallScreen (or smth), since it has nothing to do with mobile really.
| @@ -18,8 +19,24 @@ export default class PanesSplitController extends React.Component { | |||
| onSholdExpand() {} | |||
| } | |||
|
|
|||
| state = { | |||
| windowWidth: 1000 | |||
There was a problem hiding this comment.
Should't this be set to the current window.innerWidth?
There was a problem hiding this comment.
My thoughts was that window is not always available at this point. Maybe i should set it to undefined and set it on componentDidMount?
There was a problem hiding this comment.
Good point, window is available in componentDidMount. Hang on, I'm pushing a small fix in a moment.
kristofferjs
force-pushed
the
responsive-split-pane
branch
from
6153fc0
to
7cbbdb1
Compare
|
A change in window width can also be triggered by the |
bjoerge
force-pushed
the
responsive-split-pane
branch
from
4ada6c1
to
5c4a426
Compare
bjoerge
force-pushed
the
responsive-split-pane
branch
from
bdc81a2
to
4c53006
Compare
|
Did a couple of improvements:
|
…807) * [components] Switch to mobile on resize. No need to refresh browser * [components] Variable that explains its meaning * [components] Optimize screen width change detection in split pane controller * [components] Set initial windowWidth * [components] Remove redundant .startWith
…807) * [components] Switch to mobile on resize. No need to refresh browser * [components] Variable that explains its meaning * [components] Optimize screen width change detection in split pane controller * [components] Set initial windowWidth * [components] Remove redundant .startWith
Switching to responsive when resizing browser. This is nice when developing.
Before it was necessary to refresh the browser.