chore(MapTemplate): convert to TS #1265

goldpbear · 2019-04-22T19:05:22Z

Addresses: https://github.com/jalMogo/mgmt/issues/275

goldpbear · 2019-04-22T19:12:27Z

src/base/static/components/templates/map.tsx

+  mapViewport: {
+    minZoom: number;
+    maxZoom: number;
+    bearing?: number | undefined | null;


I'm not sure about these typings-- I think we need to include undefined and null because the INITIAL_STATE defined in the map duck doesn't define values for bearing, zoom, latitude, or longitude (we do this since we don't want to introduce an arbitrary initial map position or zoom).

I see a couple issues here.

It doesn't seem like we need to include both undefined and null. I think we should set it to null, and keep the type as nullable, or pick a default value. And I think it would be cleaner to explicitly define these values in our initial viewport state, like so:

initialMapViewport: { minZoom: 0, maxZoom: 18, bearing: null, // or set it to a value, and avoid making this nullable: // bearing: -60, pitch: null, // etc }

It might be out of scope of this PR, but why we are storing the INITIAL_STATE of our mapViewport in our map duck? It seems redundant to load it in app.tsx, and then read/set it to our MapTemplate component state. Can we pass the initial state as a prop from App to MapTemplate?

I think I'd rather set the map viewport state to initial numerical values, which seems like it would be cleaner.

The idea behind initialMapViewport was to keep track of both the default map viewport as set in the config, but also the viewport when navigating away from the MapTemplate. That way, the last viewport can be restored when returning to the MapTemplate after MapTemplate re-mounts.

Though we could also keep this state in App's component state, and update it via a callback from MapTemplate when it unmounts.

Though we could also keep this state in App's component state, and update it via a callback from MapTemplate when it unmounts.

Seems like a reasonable intermediate solution. But I believe a more thorough solution, however, is to use the Context API: https://reactjs.org/docs/context.html

That should give us the persistence that global state provides, without the performance drawbacks that Redux entails, while also avoiding the prop-drilling that component state entails as well.

jalMogo

Thanks for porting this module!

I left some feedback - lmk what you think 👍

jalMogo · 2019-04-22T19:29:16Z

src/base/static/components/templates/map.tsx

@@ -356,17 +430,21 @@ class MapTemplate extends Component {

  render() {
    return (
-      <>
+      <div>


Why is this changed to a div?

I suspect you might be having import issues related to the /** @jsx jsx */ pragma, which prevents Babel from transpiling the <> shorthand into <Fragment>. Note that you can work around this issue by importing Fragment and writing out <Fragment>.

jalMogo · 2019-04-22T19:57:15Z

src/base/static/components/templates/map.tsx

+  mapViewport: {
+    minZoom: number;
+    maxZoom: number;
+    bearing?: number | undefined | null;


I see a couple issues here.

It doesn't seem like we need to include both undefined and null. I think we should set it to null, and keep the type as nullable, or pick a default value. And I think it would be cleaner to explicitly define these values in our initial viewport state, like so:

initialMapViewport: { minZoom: 0, maxZoom: 18, bearing: null, // or set it to a value, and avoid making this nullable: // bearing: -60, pitch: null, // etc }

It might be out of scope of this PR, but why we are storing the INITIAL_STATE of our mapViewport in our map duck? It seems redundant to load it in app.tsx, and then read/set it to our MapTemplate component state. Can we pass the initial state as a prop from App to MapTemplate?

src/base/static/components/templates/map.tsx

Co-Authored-By: goldpbear <goldpbear@users.noreply.github.com>

goldpbear · 2019-04-23T17:32:04Z

src/base/static/components/app.tsx

@@ -169,18 +168,44 @@ declare const Mapseed: any;
 // 'process' global is injected by Webpack:
 declare const process: any;

-interface State {
+export interface IMapViewport {


Since we're exporting this interface, I thought it would be useful to start adopting the I syntax for interfaces to make it clearer when an interface is being imported into another module.

I considered using the I prefix, and decided not to do it based on some guidelines I found here: https://angular.io/guide/styleguide#interfaces and here: https://stackoverflow.com/questions/31876947/confused-about-the-interface-and-class-coding-guidelines-for-typescript/41967120#41967120

But I don't have a strong opinion on it. So feel free to stick with what you think is more comfortable. I found a full flame war on the topic here: microsoft/TypeScript-Handbook#121

Hmm, well I guess I'm not surprised it's controversial. Within a module it seems like overkill, but when exporting interfaces I thought it would make things clearer. But that's based on my one-day experience with TS.

Maybe I'll just leave it for now, and if it turns out to be silly we can revert the I syntax.

jalMogo · 2019-04-23T17:57:53Z

src/base/static/components/templates/map.tsx

@@ -12,6 +14,7 @@ import LeftSidebar from "../organisms/left-sidebar";
 import RightSidebar from "../organisms/right-sidebar";
 import GeocodeAddressBar from "../organisms/geocode-address-bar";

+import { IMapViewport } from "../app";


This is creating a circular dependency, which we should avoid: https://medium.com/content-uneditable/circular-dependencies-in-javascript-a-k-a-coding-is-not-a-rock-paper-scissors-game-9c2a9eccd4bc

I'm not sure where to put IMapViewport, but we can consider making a map.d.ts file alongside our map duck, and then import it from there.

Would it make more sense to define it in an app.d.ts file alongside the App component? The map duck doesn't store the viewport anymore, so it might be confusing.

The "d.ts" file is used to provide typescript type information about an API that's written in JavaScript, so app.d.ts wouldn't make sense.

It seems like the most proper solution here would be to put our MapViewport in a Context. Although we aren't using the Context API yet, we can consider making a new static/contexts/ dir, and putting map.tsx file under that.

Can we create something like an interfaces.ts file and put it in src/base/static instead?

My understanding of the Context API is that it's for sharing state among numerous components at different levels of the hierarchy. Since the map viewport is only used by a couple of components it seems like it might be overkill here.

My concern is that interfaces.ts would become a dumping grounds, and it's not clear what kinds of interfaces will be stored there. As we port over to TS, we're going to defining lots of interfaces.

I'm not too familiar with Context API, but it seems like a reasonable use case since we're drilling our MapViewport from App > MapTemplate > MainMap, and perhaps other components as well, like the Drawing component, Story Sidebar, etc.

But feel free to use your judgement 👍

I guess I'm not sure what the best thing to do is. Maybe to keep things simple in this PR I'll just define a separate interface in map.tsx, which actually might be appropriate because technically the initialMapViewport and mapViewport differ slightly.

And, I guess since we're not sharing interfaces I'll do away with the I syntax too :).

jalMogo

Looks good 👍

Let's make sure to fix that circular dep first before merging, though

chore(MapTemplate): convert to TS

874fb79

goldpbear commented Apr 22, 2019

View reviewed changes

goldpbear requested a review from jalMogo April 22, 2019 19:13

jalMogo reviewed Apr 22, 2019

View reviewed changes

jalMogo and others added 5 commits April 22, 2019 13:54

Update src/base/static/components/templates/map.tsx

97d9d0b

Co-Authored-By: goldpbear <goldpbear@users.noreply.github.com>

Update src/base/static/components/templates/map.tsx

2aeb7a0

Co-Authored-By: goldpbear <goldpbear@users.noreply.github.com>

refactor: review changes

4b11fed

refactor: review changes

9495db0

refactor: review changes

816a4f7

goldpbear commented Apr 23, 2019

View reviewed changes

jalMogo reviewed Apr 23, 2019

View reviewed changes

jalMogo approved these changes Apr 23, 2019

View reviewed changes

mapseed deleted a comment from modulitos Apr 23, 2019

refactor: review feedback

de6d696

goldpbear merged commit 5fd7dae into master Apr 24, 2019

goldpbear deleted the goldpbear/typescript branch May 18, 2020 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(MapTemplate): convert to TS #1265

chore(MapTemplate): convert to TS #1265

goldpbear commented Apr 22, 2019 •

edited

Loading

goldpbear Apr 22, 2019 •

edited

Loading

jalMogo Apr 22, 2019 •

edited

Loading

goldpbear Apr 22, 2019 •

edited

Loading

jalMogo Apr 22, 2019 •

edited

Loading

jalMogo left a comment

jalMogo Apr 22, 2019

jalMogo Apr 22, 2019 •

edited

Loading

goldpbear Apr 23, 2019

jalMogo Apr 23, 2019

goldpbear Apr 23, 2019

jalMogo Apr 23, 2019

goldpbear Apr 23, 2019

jalMogo Apr 23, 2019

goldpbear Apr 23, 2019

jalMogo Apr 23, 2019

goldpbear Apr 23, 2019

goldpbear Apr 23, 2019

jalMogo left a comment

chore(MapTemplate): convert to TS #1265

chore(MapTemplate): convert to TS #1265

Conversation

goldpbear commented Apr 22, 2019 • edited Loading

goldpbear Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

jalMogo Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

goldpbear Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

jalMogo Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

jalMogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalMogo Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jalMogo left a comment

Choose a reason for hiding this comment

goldpbear commented Apr 22, 2019 •

edited

Loading

goldpbear Apr 22, 2019 •

edited

Loading

jalMogo Apr 22, 2019 •

edited

Loading

goldpbear Apr 22, 2019 •

edited

Loading

jalMogo Apr 22, 2019 •

edited

Loading

jalMogo Apr 22, 2019 •

edited

Loading