Improve HOC map design by bethanyaconnor · Pull Request #30451 · code-dot-org/code-dot-org

bethanyaconnor · 2019-08-26T23:43:22Z

Update HOC map to use design Mark created. It will need to use a style created by us and hosted on Mapbox to have access to the background and icons.

This PR includes a couple of other small changes to get it ready for launch.

##OLD Description
Mapbox allows you to style a map, including the data, on their site and just use that "style". I've been using that feature to prototype designs and think we should use this in the HOC map in production (tbd on the census map).

Reasons for this change:

Because we're going to start using custom icons, we'd need to import a style anyway (easiest way to expose these custom icons)
We don't need to wait for a deploy for style changes
It allows us to download an image of the map to be served statically during hour of code

Downsides to this change:

The legend is still defined using HTML/CSS and therefore will be impossible to keep in sync if we change color schemes.
Anyone with access to the style can change the look of the map in production without review.

The biggest driver for this change is reason number 3 for me. Notably, I don't know of any way of displaying a static image instead of the map during Hour of Code without this change, other than duplicating the JS defined style in a Mapbox style and using that.

This PR also contains a change to how popups are shown for special events to be more consistent with the current map. I can separate that out if that's problematic.

…ed one

codecov-io · 2019-08-28T01:00:34Z

Codecov Report

❗ No coverage uploaded for pull request base (staging@b3bd9a3). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             staging   #30451   +/-   ##
==========================================
  Coverage           ?   72.07%           
==========================================
  Files              ?     1373           
  Lines              ?    84899           
  Branches           ?     3462           
==========================================
  Hits               ?    61191           
  Misses             ?    20461           
  Partials           ?     3247

Flag	Coverage Δ
#integration	`55.75% <ø> (?)`
#storybook	`56.56% <ø> (?)`
#unit	`58.04% <ø> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3bd9a3...5c21932. Read the comment docs.

breville · 2019-08-29T17:32:44Z

+
+      // Ensure that if the map is zoomed out such that multiple
+      // copies of the feature are visible, the popup appears
+      // over the copy being pointed to.


Not quite sure what this comment means...

The logic is correcting for the potential of a feature/point being on the map twice (when it's zoomed out enough) and ensuring that the popup appears where the click happened. I took this logic from Mapbox.

It's very easy to simplify to make it just show the popup on the spot where the user clicked but I think it looks a bit nicer for the popup to be placed on the point itself.

Ah, sounds good.

breville · 2019-08-29T17:33:45Z

+      // Ensure that if the map is zoomed out such that multiple
+      // copies of the feature are visible, the popup appears
+      // over the copy being pointed to.
+      while (Math.abs(e.lngLat.lng - coordinates[0]) > 180) {


Is it worth de-duping this with the similar logic just below?

Deduplicated a bit of the logic. There are a couple of subtle differences between the two listeners but let me know if there's anything else you'd like to see pulled out!

breville · 2019-08-29T17:34:17Z

  var map = new mapboxgl.Map({
    container: 'mapbox-map',
-    style: 'mapbox://styles/mapbox/streets-v11',
+    style: 'mapbox://styles/codeorg/cjz36duae88ds1cp7ll7smx6s',


Should we hoist this magic number into a variable with an explanation?

I pulled this out and added a brief comment but not sure what else to say.

breville · 2019-09-04T19:02:09Z


 $(document).ready(function() {
+  // We keep some style elements as a Mapbox style for simplicity.
+  const style_path = 'mapbox://styles/codeorg/cjz36duae88ds1cp7ll7smx6s';


nit: stylePath

breville · 2019-09-04T19:02:49Z

+  // copies of the point are visible, the popup appears
+  // over the point clicked on.
+  function getPopupCoordinates(clickLngLat, featureCoordinates) {
+    var normalizedCoordinates = featureCoordinates;


nit: 'let

oh, are we not going through the preprocessor for this JS file? In that case, disregard.

breville · 2019-09-04T19:03:30Z

-        .setLngLat(coordinates)
-        .setText(organization_name)
+      const popupText =
+        organization_name + (city.length > 0 ? ' (' + city + ')' : '');


nit: organizationName

…yle.

bethanyaconnor · 2019-09-04T22:01:57Z

Loading the data in JS instead of as a Mapbox style is in 5c21932. In this case we would have to screenshot the map a few days before Hour of Code (which TIL is what we've generally done). Thoughts?

breville · 2019-09-04T22:14:25Z

It sounds like taking the screenshot manually is the least burdensome approach.

bethanyaconnor · 2019-09-04T22:24:52Z

Sounds good! I've updated the description and title to reflect what this PR now does.

bethanyaconnor added 2 commits August 26, 2019 23:33

Change the HOC map to use a Mapbox hosted style instead of a JS defin…

d67ab59

…ed one

Fix legend for new colors

5bb3935

bethanyaconnor requested review from breville and nkiruka August 28, 2019 01:00

breville reviewed Aug 29, 2019

View reviewed changes

PR feedback and improving popups

93ec4f2

breville reviewed Sep 4, 2019

View reviewed changes

bethanyaconnor added 2 commits September 4, 2019 21:40

Small changes from PR feedback

87cb8bc

Load layers in the JS code instead of including them in the Mapbox st…

5c21932

…yle.

bethanyaconnor changed the title ~~Change the HOC map to use a Mapbox hosted style instead of a JS defined one~~ Improve HOC map design Sep 4, 2019

breville approved these changes Sep 4, 2019

View reviewed changes

bethanyaconnor merged commit 6b60942 into staging Sep 5, 2019

bethanyaconnor deleted the improve-hoc-map branch September 5, 2019 16:19

bethanyaconnor added the mapbox label Sep 23, 2020

Uh oh!

Conversation

bethanyaconnor commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bethanyaconnor commented Sep 4, 2019

Uh oh!

breville commented Sep 4, 2019

Uh oh!

bethanyaconnor commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bethanyaconnor commented Aug 26, 2019 •

edited

Loading

codecov-io commented Aug 28, 2019 •

edited

Loading