Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Introduce a basic URL router system #322

Merged
merged 34 commits into from
Aug 9, 2019
Merged

Conversation

bbecquet
Copy link
Contributor

@bbecquet bbecquet commented Jul 31, 2019

Description

After trying to simplify the "URL shards" system, I'm introducing a real router as a better way to manage navigation between the app states and interaction with the browser history.

This is a really simple, ad-hoc implementation, very easy to remove or replace if we decide to use something more elaborate in the future (src/proxies/app_router.js).

In this new architecture, the root component (src/panel/app_panel.js) defines URL templates with optional parameters, corresponding to valid app states, and for each template a rendering function. The panels are now only switched on or off through this function. Re-renders are the result of new URL states and not the contrary.
Transition between valid states is made explicit through a single dedicated function (navigateTo), so it happens reliably at a single place.

Why

  • the "URL shards" system was far too complicated and required logic parts scattered everywhere in the code.
  • proper browser history management was near impossible with the shards, as history pushes happened in the panel functions doing the actual renderings. Restoring past state through browser back/forward buttons resulted in new pushes. With the new system this is seamless.
  • valid app states and corresponding URLs are now explicit and grouped in a single place, making them easier to manage.
  • first step towards a single rendering flow system, which would be a better app architecture IMO.

@bbecquet bbecquet added the WIP label Jul 31, 2019
@bbecquet bbecquet force-pushed the poc-router branch 9 times, most recently from 18e93ea to 4fa40d9 Compare August 6, 2019 16:36
@bbecquet bbecquet removed the WIP label Aug 6, 2019
@bbecquet bbecquet force-pushed the poc-router branch 2 times, most recently from 6d7cf9c to 1c19109 Compare August 7, 2019 09:28
center: poi.getLngLat(),
zoom: poi.zoom,
offset,
maxDuration: 1200,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A magical value. :)

Copy link
Contributor

@amatissart amatissart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
I have just a few questions about URL parsing.

src/libs/url_utils.js Show resolved Hide resolved
src/panel/app_panel.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Contributor

Code looks good to me. Still need to check it locally though.

src/adapters/poi/poi.js Show resolved Hide resolved
src/adapters/scene.js Outdated Show resolved Hide resolved
src/panel/app_panel.js Show resolved Hide resolved
@bbecquet bbecquet merged commit 2230421 into Qwant:master Aug 9, 2019
@bbecquet bbecquet deleted the poc-router branch August 9, 2019 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants