1757 new routing #3494

soyarsauce · 2019-06-12T10:06:07Z

Adds routing such that catalog navigation can be traversed with browser history, and subsequently telling terriajshow to resolve URLs for a given catalog link

A demo of this in action with the 3 separate versions on one deployment is at (VPN required)
http://152.83.140.90:3001/catalog/54a553b4

, if you click into any of the catalog items and hit refresh you’ll see the metadata directly from the initial HTML served up, before javascript takes over and makes the rest of the UI interactive

Sitemap now autogenerated (after specifying appBaseUrl under config.json's parameters object)
http://152.83.140.90:3001/sitemap.xml

Attempts to resolve #1757

Also see:
TerriaJS/terriajs-server#106
TerriaJS/TerriaMap#342

Groups are also links now explorerPanelIsVisible now ties into location routing

kring

This is really cool! Mostly nitpicky comments below, but there are some bigger-picture things to discuss as well:

It has long been possible to deploy TerriaMap to a static web server, and we don't want to lose that ability. Does that mean providing an option to turn off the routing? Or making it (optionally?) use the hash portion of the URL instead of the path portion?
The need to specify an appBaseUrl isn't great, and is a breaking change. The Investor Map staging site, for instance, is available at both https://inv-staging.nm.terria.io/ and https://inv-staging.nm.terria.io/investormap/. NEII has something similar happening. Why do we need to know the base URL? I thought it was for the sitemap, but a quick google seems to indicate sitemaps can use relative URLs too.
As mentioned briefly in the comments below, I don't want to see the routing become yet another "source of truth". We need to be clear about where the state (whether catalog is open, which item is selected) comes from. Maybe you are, but it wasn't clear to me while looking at the code.
The mobx branch 😱. There will be conflicts. Some of this will be different there.

lib/ReactViewModels/ViewState.js

lib/ReactViews/ExplorerWindow/ExplorerWindow.jsx

kring · 2019-07-01T10:45:40Z

lib/ReactViews/DataCatalog/DataCatalogItem.jsx

+    return (
+      this.props.item.uniqueId ===
+      URI.decode(this.props.match.params.catalogMemberId)
+    );


Maybe there's a good reason for this that isn't apparent to me, but it seems to me the UI should determine which catalog item is selected based on the userDataPreviewedItem or the previewedItem properties, just as it did before. Routing should affect the values of those properties as necessary.

When the UI derives its state from multiple (potentially conflicting) places (e.g. routing and the view state), it can quickly get confusing which one is authoritative.

kring · 2019-07-01T10:49:35Z

lib/ReactViewModels/ViewState.js

+        if (!bool && this.location && this.location.pathname !== "/") {
+          setTimeout(() => {
+            this.history.push("/");
+          }, 300);


Why does this need a 300ms delay?

kring · 2019-07-01T11:07:21Z

lib/ReactViews/DataCatalog/DataCatalogItem.jsx

-module.exports = DataCatalogItem;
+module.exports = {
+  default: withRouter(DataCatalogItem),
+  DataCatalogItem: withRouter(DataCatalogItem),


Probably not a huge problem, but if I'm not mistaken calling withRouter twice will create two separate but identical functions. Better to avoid that I think.

kring · 2019-07-01T11:09:25Z

lib/ReactViews/DataCatalog/DataCatalogMember.jsx

@@ -4,7 +4,7 @@ import React from "react";
 import createReactClass from "create-react-class";
 import PropTypes from "prop-types";
 import ObserveModelMixin from "../ObserveModelMixin";
-import DataCatalogItem from "./DataCatalogItem.jsx";
+import { DataCatalogItem } from "./DataCatalogItem.jsx";


Not sure why you had to change this, since we're also exporting it as default. Maybe use ES exports for DataCatalogItem.jsx instead of mixing CJS and ES?

lib/ReactViews/Preview/DataPreviewMap.jsx

lib/ReactViews/StandardUserInterface/StandardUserInterface.jsx

soyarsauce · 2019-07-04T00:09:59Z

Thanks for bringing these up @kring

It has long been possible to deploy TerriaMap to a static web server, and we don't want to lose that ability. Does that mean providing an option to turn off the routing? Or making it (optionally?) use the hash portion of the URL instead of the path portion?

We have a couple of options. In fact with the PRs as they are, it is still possible to deploy this new configuration of TerriaMap to a static web server & have it work for basic catalogs that don't query a server for layers - it gets tricky once we start to support that too. I see a few options:

extend TerriaMap's new generate-init-routes.js rely on getting a response from the remote services, so that it can query its layers, so we know what other routes to serve up an index.html - but this might be more effort than its worth at the moment given the upcoming changes + seeing how it plays out with magda, & even then it is still kind of separate to the "backendless", "static TerriaMap"
depending on how much control a given user has over their "static web server", we simply provide configuration examples for rewriting URLs on common static web servers (apache+/.htaccess, nginx, other)
as you said, do some sort of detection or build env variable for routing on browserhistory vs hash, but this is the least ideal situation as we want proper-path-URLs for discoverability (through crawling)

The need to specify an appBaseUrl isn't great, and is a breaking change. The Investor Map staging site, for instance, is available at both https://inv-staging.nm.terria.io/ and https://inv-staging.nm.terria.io/investormap/. NEII has something similar happening. Why do we need to know the base URL? I thought it was for the sitemap, but a quick google seems to indicate sitemaps can use relative URLs too.

The appBaseUrl is actually optional & not required - and serves the purpose of

the sitemap as you say
cross-domain-canonical-references
Sitemaps can in fact be relative, I thought it wouldn't hurt to help crawlers determine which version of a duplicate deployment to use as the canonical reference. I thought about the case of same deployment to 'different hosts' but not the 'same host+different' path - are those two Investor Map staging-deployments the exact same? What would the benefit of that be?

The new default added to devserverconfig.json for baseHref of "/" is if we served up a TerriaMap that wasn't on root (e.g. on /renewables/), without knowing the baseHref we can't tell the browser the correct place to load build/TerriaMap.js from, and any subsequent relative links (e.g. /renewables/catalog/xyz)

As mentioned briefly in the comments below, I don't want to see the routing become yet another "source of truth". We need to be clear about where the state (whether catalog is open, which item is selected) comes from. Maybe you are, but it wasn't clear to me while looking at the code.

I haven't had much success without making the URL the absolute truth for these sort of things, & I tried to 'subscribe' to routing changes via RoutingListener.jsx & update viewState that way, there's certainly more room to make it clearer & I'll revisit that

The mobx branch 😱. There will be conflicts. Some of this will be different there.

......yes 😵

Visibility was not toggled correctly when waiting for animation to finish

# Conflicts: # lib/ReactViewModels/ViewState.js # lib/ReactViews/DataCatalog/CatalogItem.jsx # lib/ReactViews/SidePanel/SidePanel.jsx # lib/ReactViews/StandardUserInterface/StandardUserInterface.jsx # package.json # test/ReactViews/StandardUserInterfaceSpec.jsx

# Conflicts: # doc/customizing/client-side-config.md # lib/ReactViewModels/ViewState.js # lib/ReactViews/DataCatalog/CatalogItem.jsx # lib/ReactViews/SidePanel/SidePanel.jsx # lib/ReactViews/StandardUserInterface/StandardUserInterface.jsx # package.json # test/ReactViews/StandardUserInterfaceSpec.jsx

7.11.3

soyarsauce · 2020-04-08T10:03:14Z

At present time this is postponed indefinitely for pre-mobx(mastere), and will be fully re-implemented in mobx.

soyarsauce · 2020-04-08T10:03:54Z

(Barring merging it with even safer defaults of everything-off-by-default)

7.11.4

tephenavies · 2021-03-22T01:26:37Z

Won't be merged/implemented in version 7. PR to implement functionality in version 8 is #4961.

soyarsauce added 7 commits May 29, 2019 18:34

wip catalog routing

f16b9df

Remove duplicate link style

bfbb88d

wip catalog routing

735741a

Update routing to use const

363aef1

Add gorup routing & add graceful preview errors

db6ae8c

Groups are also links now explorerPanelIsVisible now ties into location routing

Update catalog item html titles & meta descriptions

4f8e624

Update docs for baseHref

3f5994b

soyarsauce self-assigned this Jun 12, 2019

This was referenced Jun 12, 2019

1757 tm new routing TerriaJS/TerriaMap#342

Closed

1757 ts new routing TerriaJS/terriajs-server#106

Open

soyarsauce added 6 commits June 20, 2019 19:28

Add rel="canonical" link tags for DataPreview helmet

392d214

Add prerender end event for catalog routes

6851830

Lint

cfa920e

Merge remote-tracking branch 'origin/master' into 1757-new-routing

046f546

Export non-withRouter-hoc components

6955c5c

Fix DataCatalogItemSpec selected prop tests

1a150e2

soyarsauce marked this pull request as ready for review June 26, 2019 00:14

kring requested changes Jul 1, 2019

View reviewed changes

AnaBelgun assigned kring Jul 8, 2019

soyarsauce added 10 commits August 7, 2019 12:16

Merge remote-tracking branch 'origin/master' into 1757-new-routing

9b19cb7

Track page views

b168ab7

Resolve quick minor comments from PR

444df22

Fix haeding structure

994f4b1

Fix "About This Data" & preview map reload bug

158f9ba

Fix group-selected-state & open on first load

66084ef

Fix tabs not updating when navigating

da13136

Merge remote-tracking branch 'origin/master' into 1757-new-routing

1e6fa60

Fix incorrect "About This Data" open-close flow

8435d8a

Visibility was not toggled correctly when waiting for animation to finish

Merge remote-tracking branch 'origin/master' into 1757-new-routing

6b8b521

soyarsauce added 6 commits December 2, 2019 16:33

Bump version, fix bad merge

7301a84

Lint

6515f2c

Update add user data button (bandage)

40f4351

Bump version

5da6ad6

tephenavies unassigned kring Jan 14, 2020

Merge tag '7.11.3' into 1757-new-routing

5ea5986

7.11.3

soyarsauce mentioned this pull request Mar 26, 2020

Google/Console Analytics: some maps don't want full path name #4164

Open

soyarsauce added the Low priority label Apr 8, 2020

reginapramesti and others added 5 commits April 17, 2020 13:51

Merge tag '7.11.4' into 1757-new-routing

e77fa4b

7.11.4

Run prettier

aeb6e1d

Create prerender-troubleshooting.md

1f3d1f7

feedback form is now scrollable

7c89cde

new version 7.4.11-prerender-1

d5fe95d

soyarsauce mentioned this pull request Sep 3, 2020

1757 mobx new routing #4679

Merged

2 tasks

tephenavies closed this Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1757 new routing #3494

1757 new routing #3494

soyarsauce commented Jun 12, 2019 •

edited

Loading

kring left a comment

kring Jul 1, 2019

kring Jul 1, 2019

kring Jul 1, 2019

kring Jul 1, 2019

soyarsauce commented Jul 4, 2019

soyarsauce commented Apr 8, 2020

soyarsauce commented Apr 8, 2020

tephenavies commented Mar 22, 2021

1757 new routing #3494

1757 new routing #3494

Conversation

soyarsauce commented Jun 12, 2019 • edited Loading

kring left a comment

Choose a reason for hiding this comment

kring Jul 1, 2019

Choose a reason for hiding this comment

kring Jul 1, 2019

Choose a reason for hiding this comment

kring Jul 1, 2019

Choose a reason for hiding this comment

kring Jul 1, 2019

Choose a reason for hiding this comment

soyarsauce commented Jul 4, 2019

soyarsauce commented Apr 8, 2020

soyarsauce commented Apr 8, 2020

tephenavies commented Mar 22, 2021

soyarsauce commented Jun 12, 2019 •

edited

Loading