Add fullscreen option to map #1655

pezholio · 2017-03-01T13:17:01Z

For mysociety/fixmystreetforcouncils#156

This adds a 'Show Fullscreen' option to the map on a /report page to make the map easier to use when on mobile:

dracos

This is lovely 🍇 On the other hand, I've potentially asked you to change a lot of it ;)

dracos · 2017-03-01T14:27:18Z

web/cobrands/fixmystreet/fixmystreet.js

@@ -869,6 +869,10 @@ $.extend(fixmystreet.set_up, {
        }
        $('#key-tools li:empty').remove();
        $('#report-updates-data').insertAfter($('#map_box'));
+        if (fixmystreet.page === 'report') {
+            $('#sub_map_links').append('<a href="#" id="show-fullscreen" class="expand">Expand map</span>');


These need internationalizing, see how other strings do it elsewhere :)

dracos · 2017-03-01T14:32:43Z

web/cobrands/sass/_base.scss

@@ -1496,6 +1497,48 @@ html.js #map .noscript {
      background-image:url($image-sprite);
      background-position: flip(right, -341px) -3936px;
    }
+    &.expand {
+      padding: 0.6em 1em 0.5em 1em;
+      &:after {


Not saying this is wrong, but why is this done differently to the other sub map link icons, with an :after, and not just an immediate background-image?
Also, note how the other icons do their positioning - you need to account for the LTR/RTL possibility (where you'd want the icon on the right in RTL).

Was wrestling with this for a while this morning (my CSS-fu is weak), and @zarino suggested doing it this way, as positioning relative to the right hand side is tricky to do with background-position. I think it's easier with the others as they use a sprite, but I decided to use an SVG as we seem to be moving in that direction for other stuff.

Fair enough! Hmm, not sure the LTR/RTL code has had to deal with an after/before swap before, I'll leave that with you to try and sort ;) There are many uses of the left/right and flip() mixins that hopefully make sense. .expand and .close can share most of their CSS too.

Yup, my fault! Maybe we’d use the sprite more, if we had some convenience functions to automatically compile them and surface coordinates for positioning in the sass.

Re: RTL, my understanding is that the :after pseudo element will respect RTL ordering, if it’s display: inline or display: inline-block – ie: in English, the pseudo-element will sit to the right of the button, in Arabic it'd sit to the left. I think.

You’d just need to make sure you use our RTL sass mixins like flip() for any margins/padding you added.

dracos · 2017-03-01T14:33:45Z

web/cobrands/fixmystreet/fixmystreet.js

@@ -869,6 +869,10 @@ $.extend(fixmystreet.set_up, {
        }
        $('#key-tools li:empty').remove();
        $('#report-updates-data').insertAfter($('#map_box'));
+        if (fixmystreet.page === 'report') {
+            $('#sub_map_links').append('<a href="#" id="show-fullscreen" class="expand">Expand map</span>');
+            $('#sub_map_links').append('<a href="#" id="hide-fullscreen" class="compress hidden">Collapse map</span>');


Would this be nicer as one link that changes class/text when clicked, rather than two? Guess it doesn't make much difference, but I think other similar uses in the code currently toggle the one link.

dracos · 2017-03-01T14:34:19Z

web/cobrands/sass/_base.scss

@@ -1559,7 +1602,7 @@ html.js #map .noscript {
  text-transform: uppercase;
  text-align: center;
  font-size: 0.875em;
-  .mobile-reporting-map & {
+  .mobile-reporting-map &, .report-fullscreen & {


This class never appears on a report page.

dracos · 2017-03-01T14:50:43Z

web/cobrands/sass/_base.scss

@@ -1656,7 +1699,7 @@ a:hover.rap-notes-trigger {
 // and make the map full screen to reduce distractions. JavaScript also
 // tweaks the text content of some of the map-related elements, to make
 // it more "app-like".
-.mobile-reporting-map {
+.mobile-reporting-map, .report-fullscreen {


I like how these share a lot, I wonder if we can clear it up a bit to split it out a bit more. So we'd have:

one class that shows the map full screen, map-fullscreen perhaps;

one class that 'locks' it, that's only-map;

one class that does page-specific thing, so mobile-reporting-map (perhaps renamed something like map-reporting?) and its pan zoom top.

Then the report page would have map-fullscreen only-map, and the around page would have map-fullscreen only-map map-reporting (where it currently has mobile-reporting-map only-map). Does that make sense, or not worth it?

Yeah, I like this, then if we want to use map-fullscreen anywhere else, it makes sense, without having to add another class

dracos

Looking good, a couple of small things.

dracos · 2017-03-02T11:33:36Z

web/cobrands/sass/_base.scss

+        }
+        background-size: 16px 16px;
+        vertical-align: middle;
+        margin: flip(0 0 0 8px, 0 8px 0 0);


This is fine, just to note that if you did only want to do the one margin as originally, you could write it as:
margin-#{$left}: 8px;.

Aaaah, shiny!

dracos · 2017-03-02T11:36:56Z

web/cobrands/fixmystreet/fixmystreet.js

@@ -869,7 +869,7 @@ $.extend(fixmystreet.set_up, {
        }
        $('#key-tools li:empty').remove();
        $('#report-updates-data').insertAfter($('#map_box'));
-        if (fixmystreet.page === 'report') {
+        if (['reports','report'].includes(fixmystreet.page)) {


includes is much too modern, I'm afraid.

dracos · 2017-03-02T11:40:21Z

web/cobrands/sass/_base.scss

+        margin: flip(0 0 0 8px, 0 8px 0 0);
+      }
+    }
+    &.compress {


.expand and .compress still share most of their style, all of it apart from the image URLs could be on #toggle-fullscreen for example.

pezholio added the In progress label Mar 1, 2017

dracos added Reviewing and removed In progress labels Mar 1, 2017

pezholio requested a review from davea March 1, 2017 13:17

pezholio force-pushed the issues/forcouncils/156-add-fullscreen-option-to-map branch from 66dbd19 to 1896c34 Compare March 1, 2017 13:21

dracos requested changes Mar 1, 2017

View reviewed changes

dracos added Reviewed and removed Reviewing labels Mar 1, 2017

pezholio requested a review from dracos March 1, 2017 16:24

pezholio mentioned this pull request Mar 1, 2017

Shortlist multiple reports #1652

Merged

dracos requested changes Mar 2, 2017

View reviewed changes

pezholio requested a review from dracos March 2, 2017 15:47

dracos approved these changes Mar 2, 2017

View reviewed changes

pezholio force-pushed the issues/forcouncils/156-add-fullscreen-option-to-map branch from f6df291 to 664850e Compare March 2, 2017 16:17

pezholio closed this Mar 2, 2017

pezholio force-pushed the issues/forcouncils/156-add-fullscreen-option-to-map branch from 664850e to a38b5d7 Compare March 2, 2017 16:25

dracos removed the Reviewed label Mar 2, 2017

dracos deleted the issues/forcouncils/156-add-fullscreen-option-to-map branch April 13, 2017 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fullscreen option to map #1655

Add fullscreen option to map #1655

pezholio commented Mar 1, 2017

dracos left a comment

dracos Mar 1, 2017

dracos Mar 1, 2017

pezholio Mar 1, 2017

dracos Mar 1, 2017

zarino Mar 1, 2017

dracos Mar 1, 2017

dracos Mar 1, 2017

dracos Mar 1, 2017

pezholio Mar 1, 2017

dracos left a comment •

edited

dracos Mar 2, 2017

pezholio Mar 2, 2017

dracos Mar 2, 2017

dracos Mar 2, 2017

Add fullscreen option to map #1655

Add fullscreen option to map #1655

Conversation

pezholio commented Mar 1, 2017

dracos left a 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

dracos left a comment • edited

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

dracos left a comment •

edited