Estimate initial bbox from url points, disable zoom animation#171
Estimate initial bbox from url points, disable zoom animation#171
Conversation
| if (initialBBox) { | ||
| const updatedState = calculateLatLngFromBbox(this.state, initialBBox, isSmallScreenQuery()) | ||
| super.setState(updatedState) | ||
| } |
There was a problem hiding this comment.
@Janekdererste I wasn't sure how else I shoud do this. I had to set Store#setState public for this. The problem is that getInitialState is called in the super-constructor and subclasses like ViewportStore have no chance to make use of the constructor parameters before. Here I'd like to pass the initialBBox via the constructor and actually make use of it to set the initial state.
There was a problem hiding this comment.
Since the only way to alter a store's state should be through actions, I guess you need an action for this. Like UpdateBboxAction.
Initialize all the stores in index.ts then parse the bbox and issue an UpdateBboxAction to the Dispatcher.
There was a problem hiding this comment.
Ok, I did this in my last commit.
| width: window.screen.width, | ||
| height: window.screen.height, |
There was a problem hiding this comment.
Using the window.screen.height/width here is useful to get a rough estimation of the bounding box size, but maybe it could be more precise. The windows size is not equal to the map size and I think this is the reason why we still see a small flickering of the map: First the map is zoomed such that the markers are right at the map borders and then zoomed out a bit more. @Janekdererste do you have an idea how we could improve this?
There was a problem hiding this comment.
I think this is a reasonable heuristic, since the map is pretty much the same size as the window. This gets called before the first render pass and before any css is applied though. Therefore, it is not really possible to guess the map size correctly
Janekdererste
left a comment
There was a problem hiding this comment.
I would think that some of this will become obsolete once the viewport information is stored in the map again. I'd assume that this is the case once you merge your PR with the switch to openlayers.
| abstract reduce(state: TState, action: Action): TState | ||
|
|
||
| private setState(value: TState) { | ||
| public setState(value: TState) { |
There was a problem hiding this comment.
I don't think we want to have this public.
There was a problem hiding this comment.
Ok yes it's no longer needed since I use an action to set the initial state now.
| if (initialBBox) { | ||
| const updatedState = calculateLatLngFromBbox(this.state, initialBBox, isSmallScreenQuery()) | ||
| super.setState(updatedState) | ||
| } |
There was a problem hiding this comment.
Since the only way to alter a store's state should be through actions, I guess you need an action for this. Like UpdateBboxAction.
Initialize all the stores in index.ts then parse the bbox and issue an UpdateBboxAction to the Dispatcher.
| return url.searchParams.has('key') ? url.searchParams.get('key') : config.keys.graphhopper | ||
| } | ||
|
|
||
| function parseURLBBox() : Bbox | null { |
There was a problem hiding this comment.
Ultimately this could be in the NavBar component as well. Once you update the viewport state by dispatching an action, you could put this logic there.
| width: window.screen.width, | ||
| height: window.screen.height, |
There was a problem hiding this comment.
I think this is a reasonable heuristic, since the map is pretty much the same size as the window. This gets called before the first render pass and before any css is applied though. Therefore, it is not really possible to guess the map size correctly
… info bbox for now (graphhopper#171)
Following the discussion in #168 here I disabled the bbox update after the
/inforequest and disabled the zoom animation. For URLs with points I estimate the bounding box of the resulting/routerequest to prevent loading the world view just before we zoom to the route. This should improve the map/page loading performance because less tiles need to be loaded (#132).