Added a basic menu for mobile #72

coltonb · 2017-01-25T17:41:50Z

To try and alleviate the issue presented in #66, I added simple show/hide menu js functions, added a mobile menu icon, and added CSS rules to properly display based on whether the user is on mobile or desktop.

RitwikGupta

This is amazing! Great work!

vonbearshark · 2017-01-25T18:04:52Z

Waitwait! It turns out we actually want to be able to show and hide these on Desktop, too. Also, I've got some feedback for the code itself. I'll review it now

RitwikGupta · 2017-01-25T18:06:03Z

Actually, after merging so I could open it on my phone for real instead of Chrome dev tools, clicking the menu bar does nothing on iOS (iPhone 7)

brlodi · 2017-01-25T18:06:24Z

Yeah the hamburger menu doesn't actually work in mobile Safari

brlodi · 2017-01-25T18:06:37Z

^ we crossed comments

brlodi · 2017-01-25T18:07:55Z

Also I'm not sure testing by merging into a branch that's automatically pushed to production is a sustainable workflow @RitwikGupta

RitwikGupta · 2017-01-25T18:10:01Z

@brlodi I came, I merged, I regretted

brlodi · 2017-01-25T18:13:37Z

Veni, insedi, paenitui

coltonb · 2017-01-25T18:13:45Z

I was only using chrome's device toolbar to test, my bad!

vonbearshark

Added some Review, but we can keep working back and forth on this

vonbearshark · 2017-01-25T18:08:34Z

index.html

+      	    </ul>
+      	  </div>
+      	</div>
+        <div id="mobileMenuIcon"></div>


Am I crazy or is this one space off to the right?

It turns out we want to expand/contract on desktop, too, so we want to show this either way, and we should avoid the mobile terminology/design.

Instead of a hamburger icon, maybe we could have a small right-pointing arrow that expands onclick and turns into a left-pointing arrow in the top right corner of the UI that contracts onclick.

Also, it seems broken on some mobile browsers, and at least for me the hide is never shown on the UI after the icon is clicked

I believe the brokenness on iOS comes from some weird issue where it doesn't register click events on tags that don't typically have them, i.e. a div. I'm going to use an <a href="#"> instead, should hopefully fix the issue.

How about a button?

Safari error console is showing this after clicking on the hamburger button

I was going to suggest trying to set #sidebar or something to display:none, but can you not set display on Safari?

I believe it's reporting document.getElementById().style as a readonly property, since it's a JS error.

Could it be this? http://stackoverflow.com/questions/24906279/how-to-set-element-style-property-in-strict-mode?answertab=votes#tab-top

I suspect that's indeed the issue.

vonbearshark · 2017-01-25T18:12:44Z

main.css

+/*For Desktop*/
+@media only screen and (min-width: 800px)
+{
+    #mobileMenuIcon


We want to show on the desktop, too, so we can show it regardless

vonbearshark · 2017-01-25T18:17:56Z

main.css

@@ -94,15 +94,56 @@ footer
    background-color: #061428;
 }

-#sidebar
+#mobileMenuIcon


Again, we should call this something different since we want it on desktop, too. Maybe expandIcon and/or contractIcon, expandContractIcon, etc.

vonbearshark · 2017-01-25T18:20:08Z

main.css

+    width: 48px;
+    height: 48px;
+    z-index: 9999;
+    background: url("assets/mobile-ui/mobilemenu.png");


We might want to use fontawesome (http://fontawesome.io/icons/) for this. Some overhead, but we might need them for other parts of the UI. Maybe we can just pull them as we need them, or point this URL to a remote endpoint

Sounds good, just threw something together and didn't want to violate copywrite law haha.

@vonbearshark The pin iconography is actually from an icon font too, so if we do go the icon font route we'd be pretty well set up for divIcon pins in future.

Do you want to set that up, @brlodi? Ref is #25, right? I can assign you

You can assign me, but there's some discussion to be had about which icon font to use (FontAwesome actually has very limited relevant iconography, which I know because I started there when building the pin assets)

Could we use fontello to bundle only the ones we need? http://fontello.com/

Oh god I'm going to be lost in the Fontello rabbit hole forever now. (yes)

vonbearshark · 2017-01-25T18:23:12Z

main.css

+    background: url("assets/mobile-ui/mobilemenu.png");
+}
+
+#mobileHideMenu


This never seems to show up for me? Either way, we should have arrows instead, and probably use the same element for both maybe a #toggleExpandContact div that can be given/taken away the #expandIcon/#contractIcon id

There's no explicit button as I'm using the grayed out area as the return button for now, but will do.

vonbearshark · 2017-01-25T18:24:11Z

main.css

-    width: 250px;
-    height: 500px;
+    width: 65%;
+    height: 100%;


Like this change a lot

vonbearshark · 2017-01-25T18:26:55Z

main.js

@@ -55,11 +55,28 @@
        });
    }

+    //Displays and hides the sidebar for mobile
+    function displaySidebar() {
+      document.getElementById("sidebar").style = "display:block";


since we're converting this to being one element that simply changes the direction of the arrow, we need to:

Display the sidebar

Change the element to having the opposite arrow (via id substitution, ideally, rather than setting .style directly)

Change the element's onclick to hideSidebar

vonbearshark · 2017-01-25T18:27:14Z

main.js

+    }
+
+    function hideSidebar() {
+      document.getElementById("sidebar").style = "";


since we're converting this to being one element that simply changes the direction of the arrow, we need to:

Display the sidebar

Change the element to having the opposite arrow (via id substitution, ideally, rather than setting .style directly)

Change the element's onclick to displaySidebar

vonbearshark · 2017-01-25T18:28:49Z

main.js

    //Add listeners for radio buttons
    document.getElementById("radioDay").addEventListener("click", displayPastDay);
    document.getElementById("radioWeek").addEventListener("click", displayPastWeek);
    document.getElementById("radioMonth").addEventListener("click", displayPastMonth);

+    //Listeners for mobile menu
+    document.getElementById("mobileMenuIcon").addEventListener("click", displaySidebar);
+    document.getElementById("mobileHideMenu").addEventListener("click", hideSidebar);


If mobile, should start hidden and click should display, otherwise should start shown and click should hide. Need to check for that and set all that up, depending.

coltonb · 2017-01-25T18:44:09Z

I'm gonna give this all a try in a few hours, thanks for the feedback @vonbearshark

Added a basic menu for mobile

015ce8b

RitwikGupta approved these changes Jan 25, 2017

View reviewed changes

RitwikGupta merged commit b7b8d83 into pittcsc:master Jan 25, 2017

RitwikGupta mentioned this pull request Jan 25, 2017

Revert "Added a basic menu for mobile" #73

Closed

vonbearshark reviewed Jan 25, 2017

View reviewed changes

Added a basic menu for mobile #72

Added a basic menu for mobile #72

Conversation

coltonb commented Jan 25, 2017

RitwikGupta left a comment

Choose a reason for hiding this comment

vonbearshark commented Jan 25, 2017

RitwikGupta commented Jan 25, 2017

brlodi commented Jan 25, 2017

brlodi commented Jan 25, 2017

brlodi commented Jan 25, 2017

RitwikGupta commented Jan 25, 2017

brlodi commented Jan 25, 2017

coltonb commented Jan 25, 2017

vonbearshark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coltonb Jan 25, 2017 • 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

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

coltonb commented Jan 25, 2017

coltonb Jan 25, 2017 •

edited

Loading