Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sn 4 mobile nav styles #906

Merged
merged 19 commits into from
Nov 23, 2015
Merged

Sn 4 mobile nav styles #906

merged 19 commits into from
Nov 23, 2015

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Sep 17, 2015

Refiling #867 against develop, for merging purposes.

Changes

  • Carets in the mobile nav point right before drop-downs are opened, and then point down.
  • Third-level and fourth-level menu items are hidden more forcefully
  • Highlights on active and hovered menu items are removed on mobile only
  • Menu hamburger becomes dark on light, using the standard link color for the bars. Previously, this had used the link color for the box and white for the bars.
  • Text and carets in sticky nav are now all using the standard link color.
  • Remove all styles pertaining to .nav-middle and .nav-collapse, which were not found in any Largo HTML, PHP, CSS, or JavaScript.

Checks

Here's the list of sites on production, and whether their themes have been checked or not: https://docs.google.com/spreadsheets/d/11VMk3kMz1S_eGzkJj6c8_MBlHqZFX6AssfpngrnB2Zs/edit#gid=0

This also includes a general overview of trouble points, with my best guess at coloration for whether things are normal 📗, something to look out for 💛, or something to actively check for 🔴.

Themes marked as gtg don't need any theme changes for this PR, but may need the Custom CSS recompile button pressed, which is in a separate column.

Themes marked deploy have a branch largo-sn-4-updates that will need to be merged into master.

Time for this is being tracked in http://jira.inn.org/browse/LAR-13

To do

Why

For SN-4 and #807. (See also #732)

@aschweigert aschweigert added this to the 0.5.4 - Header/Navigation milestone Sep 18, 2015
@rnagle
Copy link

rnagle commented Sep 18, 2015

@benlk Testing this and noting that I can't actually click to follow the parent link for drop down menus. For example, in the screenshot below, I can click to expand/collapse "Uncategorized" but I can't actually go to the "Uncategorized" category page:

image

We'll want to address this before merging.

@benlk
Copy link
Collaborator Author

benlk commented Sep 18, 2015

Oh, good catch. i was only testing this with the top-level item as a custom item linking to #

@benlk
Copy link
Collaborator Author

benlk commented Sep 21, 2015

This is because of something that's been in Largo for a while: the click on the a gets hijacked by the dropdown javascript that's listening for a click on the .dropdown-toggle, which is the same element as the a. If I'm reading this code correctly, no top-level menu item should be used as a link, because it will always be used as a menu toggle on mobile: https://github.com/INN/Largo/blob/SN-4-mobile-nav-styles/js/largoCore.js#L350-L368

Possible fixes:

  • Tell people to not use the root of a drop-down as a link (not doable)
  • Switch it to search for clicks on the parent li, not the a
  • Listen for clicks on the b.caret, not the a

@aschweigert
Copy link

probably listen for clicks on the caret

@benlk
Copy link
Collaborator Author

benlk commented Sep 21, 2015

My worries for clicks on the caret are:

  • The caret is a tiny touch target (but can be made larger with CSS, so shrug)
  • Unless the caret has a distinct border, I think people are going to interpret as an indicator rather than a distinct button that needs to be pressed to achieve dropdown functionality
  • People are still probably going to try to click on the bar to get the link, and the a is the full width of the bar, so people will probably mistakenly click on the link instead of the caret.

@aschweigert
Copy link

As long as the touch target is sufficiently large (44px is the de facto standard per Apple's human interface guidelines) I think it's fine to just use the caret as the target. @kaeti should also weigh in on this.

@benlk
Copy link
Collaborator Author

benlk commented Sep 21, 2015

Latest commit bumps it up to 50px square, but it's still transparent. It also uses a few negative margins to make sure that it stays centered. 😢

@kaeti
Copy link

kaeti commented Sep 21, 2015

Yeah, if people are going to use top level categories as links in this nav, just use the caret, but add some extra padding to that click target for those of us that are all thumbs.

@benlk
Copy link
Collaborator Author

benlk commented Sep 21, 2015

screen shot 2015-09-21 at 2 58 50 pm

screen shot 2015-09-21 at 2 58 59 pm

@aschweigert
Copy link

That looks like it might be a bit on the large side, can you take a slightly larger screenshot so we can see a little more context? I think probably 50px sq or so should be fine

@benlk
Copy link
Collaborator Author

benlk commented Sep 21, 2015

Here's what Chrome tells me is an iPhone 6: 375px wide:

screen shot 2015-09-21 at 3 50 34 pm

screen shot 2015-09-21 at 3 51 08 pm

The padding and margin are left over from the desktop styles, but don't really affect the display here. They can be removed.

screen shot 2015-09-21 at 3 52 33 pm

screen shot 2015-09-21 at 3 52 41 pm

@benlk
Copy link
Collaborator Author

benlk commented Sep 23, 2015

@aschweigert @kaeti Is this big enough?

@aschweigert
Copy link

that seems fine to me

@kaeti
Copy link

kaeti commented Sep 23, 2015

👍

On Sep 22, 2015, at 7:14 PM, Adam Schweigert notifications@github.com wrote:

that seems fine to me


Reply to this email directly or view it on GitHub.

@benlk
Copy link
Collaborator Author

benlk commented Sep 23, 2015

👍

@benlk benlk mentioned this pull request Oct 29, 2015
3 tasks
@aschweigert aschweigert modified the milestones: 0.5.4 - Header/Navigation, 0.5.3 - Widgets Oct 30, 2015
rnagle added a commit that referenced this pull request Nov 23, 2015
@rnagle rnagle merged commit 4fd4b3f into develop Nov 23, 2015
@benlk benlk deleted the SN-4-mobile-nav-styles branch February 3, 2016 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants