Fix navigation in mobile devices #702

jaguarfi · 2020-11-02T13:17:38Z

Description

In the issue #627 it was reported that the use of the toggle button for mobile devices was breaking the landmarks and logical of the page content. In addition, some other small issues were addressed in this PR.

The button "skip navigation" of the left side course menu was present in small devices and it was overlapping other elements inside the toggle menu. Therefore, that specific "skip navigation" link was removed in small devices. It is not necessary to have a "skip navigation" button inside the toggle menu.
The tab navigation order of the navbar elements in mobile devices was in the wrong order. Therefore, the elements were arranged to follow a logical flow from left to right.
The focus styles were not applied to the toggle button. Therefore, the styles were applied to the toggle button. One for keyboard navigation and other for the click/touch interaction.
The contrast of the links located inside the toggle menu was low. Therefore, the color property was changed.
The brand button was not interactive in small devices. Therefore, the brand button interaction was added to small devices.

Existing

NEW Changes

Fixes #627

Testing

The changes were testes manually.

Have you updated the README or other relevant documentation?

...

Is it Done?

Clean up your git commit history before submitting the pull request!

I (developer) have created unit tests and the tests pass
I (developer) have created functional tests (Selenium tests) if applicable
I (developer) have tested the changes manually
Reviewer has finished the code review
After the review, the developer has made changes accordingly
Customer/Teacher has accepted the implementation of the feature

markkuriekkinen

Nice! There are some issues to fix.

very low contrast: mobile menu, focus during keyboard tabbing, yellow background and white font. Font color must be changed in the focus style.
very low contrast: gray "course staff" text (not link) in the mobile menu. Also very small font size warning. The same for text "Course" and "Site" in the mobile menu.
"Course" text is not well aligned with the language button
The language selection menu (Finnish/English) does not look good in the mobile menu. You must fix the background and font colours. You can set language "|en|fi|" in the A+ course settings even if the course is only defined in one language like the aplus-manual.

markkuriekkinen · 2020-11-05T06:11:49Z

course/templates/course/_course_menu.html

@@ -8,7 +8,7 @@
 {% prepare_course_menu %}

 <li>
-	<a href="#course-content" class="skip-link page-skip-link">
+	<a href="#course-content" class="skip-link page-skip-link hidden-xs">


Does this mean that mobile users don't have any skip links (except the first skip link for skipping all navigation)? The skip links are intended for keyboard users and small mobile devices don't have any keyboards, so it would make sense in that regard. I wonder if there is any accessible technology that would be used with small screens and the mobile versions of web sites. Can you find out whether skip links are usually included even in mobile web pages?

After testing this myself, I think it works well right now. The mobile course menu is easy to skip with the tab key because the menu does not open unless you hit the Enter key. If you don't open the mobile menu, pressing tab skips the whole menu without needing any skip link.

Can you verify how screen readers behave with the mobile course menu? What does it say on top of the mobile menu open button?

The language menu must be also tested with the screen reader.

It seems like mobile devices have certain features that allow users to navigate websites with certain gestures.
https://developer.mozilla.org/en-US/docs/Learn/Accessibility/Mobile. But, I was also checking some websites, and they keep the main "skip navigation" link in small devices. You can check in bootstrap, GOV.UK and GitHub websites.

Can you verify how screen readers behave with the mobile course menu? What does it say on top of the mobile menu open button?

It says "toggle navigation button collapsed" if the menu is closed or "toggle navigation button open" if it is open. I think that is the expected behaviour, or at least that message is used also in bootstrap and Github webpages.

The language menu must also be tested with the screen reader.

Which language menu? This?

If so, I tested the behaviour of that button in production, and it works as intended, but we can take a closer look at that button when solving this issue #428

Yes, I mean the language menu that selects between Finnish and English.

markkuriekkinen · 2020-11-05T07:00:12Z

templates/base.html

@@ -79,6 +79,10 @@
                <nav class="topbar navbar navbar-inverse navbar-static-top" aria-label="{% trans "Main" %}">
                    <div class="container-fluid">
                        <div class="navbar-header">
+							<a class="navbar-brand" href="{% url 'home' %}">{% brand_name %}</a>


What does this change do? Would you clarify in the discussion here?

The brand button had an anchor pointing to the home page, but the anchor is removed for small devices, and that makes no sense. Users in small devices also expect the brand button to behave as a link to the home page. That is the reason I remove the code below. The code that is shown in green already existed, but the changes in indentation makes the code looks like a new piece of code. I only replaced the hidden-xs class with the visible-xs class, and move it the brand button before the toggle menu.

markkuriekkinen · 2020-11-05T07:12:39Z

templates/base.html

-                            <a class="navbar-brand hidden-xs" href="{% url 'home' %}">{% brand_name %}</a>
-                            <span class="navbar-brand visible-xs">
-                            {% brand_name %}{% if instance %} &nbsp; {{ course.name|parse_localization }}{% endif %}
-                            </span>
                        </div>
                        <div class="collapse navbar-collapse" id="bs-navbar-collapse">
                            <ul class="nav navbar-nav hidden-xs">


I wonder if the course instance selection menu could be shown in the mobile view too. Could you try this?

I think we can include the drop-down menu in the toggle menu.

What about this?

Or we can keep the dropdown menu next to the brand icon just as in the desktop version.

@Mankro which one do you like more?

I like the second one (separate course instance menu and the toggle menu like in the desktop version).

jaguarfi · 2020-11-13T12:25:16Z

I applied the requested changes

jaguarfi · 2020-11-13T12:33:31Z

@Mankro I have one commit that format the base.html file, so I think you will have to review the changes by commit :)

jaguarfi · 2020-11-13T14:54:16Z

There is something I needed to fix!!

jaguarfi · 2020-11-13T15:40:09Z

Now is ready 👍🏽

Fixes apluslms#627

Currently, in small devices it is not possible to change the course instance directly from the navbar. This commit add a dropdown menu with the course instances for small devices.

markkuriekkinen

Excellent work! I will make a new issue about too low contrast in the new notification dropdown of the mobile course menu. Let's merge this now.

markkuriekkinen · 2020-11-19T15:27:40Z

@jaguarfi new small issue about low contrast: #723

jaguarfi requested a review from markkuriekkinen November 2, 2020 13:19

jaguarfi force-pushed the fix_627 branch from d31a973 to 7468f32 Compare November 2, 2020 13:32

markkuriekkinen suggested changes Nov 5, 2020

View reviewed changes

jaguarfi requested a review from markkuriekkinen November 10, 2020 13:09

jaguarfi marked this pull request as draft November 13, 2020 14:53

jaguarfi marked this pull request as ready for review November 13, 2020 15:36

jaguarfi force-pushed the fix_627 branch from 23f4efb to d8b721f Compare November 13, 2020 15:37

jaguarfi added 4 commits November 14, 2020 20:12

Fix course navigation for small devices

5b296ea

Fixes apluslms#627

Add course instance selector to small devices

e62ecaf

Currently, in small devices it is not possible to change the course instance directly from the navbar. This commit add a dropdown menu with the course instances for small devices.

Modify language selector for small devices and format _course_menu.html

ce63919

Format /templates/base.html

72174b1

jaguarfi force-pushed the fix_627 branch from d8b721f to 72174b1 Compare November 14, 2020 20:34

markkuriekkinen approved these changes Nov 19, 2020

View reviewed changes

markkuriekkinen merged commit 22a14c0 into apluslms:master Nov 19, 2020

markkuriekkinen mentioned this pull request Nov 19, 2020

Low contrast in the notification dropdown in the mobile course menu #723

Closed

jaguarfi mentioned this pull request Nov 20, 2020

Modify color of dropdown items in the topbar for small devices #724

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix navigation in mobile devices #702

Fix navigation in mobile devices #702

jaguarfi commented Nov 2, 2020 •

edited

Loading

markkuriekkinen left a comment

markkuriekkinen Nov 5, 2020

markkuriekkinen Nov 5, 2020

jaguarfi Nov 9, 2020

jaguarfi Nov 9, 2020

markkuriekkinen Nov 9, 2020

markkuriekkinen Nov 5, 2020

jaguarfi Nov 9, 2020 •

edited

Loading

markkuriekkinen Nov 5, 2020

jaguarfi Nov 9, 2020 •

edited

Loading

jaguarfi Nov 9, 2020

markkuriekkinen Nov 9, 2020

jaguarfi commented Nov 13, 2020

jaguarfi commented Nov 13, 2020

jaguarfi commented Nov 13, 2020 •

edited

Loading

jaguarfi commented Nov 13, 2020

markkuriekkinen left a comment

markkuriekkinen commented Nov 19, 2020

Fix navigation in mobile devices #702

Fix navigation in mobile devices #702

Conversation

jaguarfi commented Nov 2, 2020 • edited Loading

Description

Existing

NEW Changes

Testing

Have you updated the README or other relevant documentation?

Is it Done?

markkuriekkinen 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

jaguarfi Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaguarfi Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaguarfi commented Nov 13, 2020

jaguarfi commented Nov 13, 2020

jaguarfi commented Nov 13, 2020 • edited Loading

jaguarfi commented Nov 13, 2020

markkuriekkinen left a comment

Choose a reason for hiding this comment

markkuriekkinen commented Nov 19, 2020

jaguarfi commented Nov 2, 2020 •

edited

Loading

jaguarfi Nov 9, 2020 •

edited

Loading

jaguarfi Nov 9, 2020 •

edited

Loading

jaguarfi commented Nov 13, 2020 •

edited

Loading