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

Points popover and overflow #9616

Merged

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Aug 15, 2022

Summary

This PR continues updates to the AppBar, making changes to the display of points and managing the overflow of navigation tabs in a new dropdown menu, rather than a horizontal overflow with scroll.

References

Fixes #9276
Fixes #9277

Points display

➡️ Note: out of scope was updating the icon, as I believe the icon design wasn't finalized at the time of dev handoff.

Spec
image
Implementation
points-popover

@radinamatic I did some preliminary testing using keyboard navigation, and also using Mac voiceover, and based on my very basic understanding, it seems functional 😄 but I know things are not always that straight forward. I added an aria label to the icon button, but if there are additional changes that can improve any parts of the accessibility experience, please do let me know. One other consideration I had thought about was - would there be any pros/cons to not having the popover for screenreader users, and rather directly having the aria label just read aloud the points information? I'm not sure if there is an easy way to do this or if there is WCAG guidance on these sorts of things.

Overflow display

The overflow display creates a new component which is responsible for checking the viewport width, and managing overflow by itself. It receives an array of navigation links. I think there is potential for further refactoring here, potentially to replace the intermediary [Plugin]TopNav entirely in a lot of places, but, I wanted to think/talk that through before jumping in. Would be happy to open and take on a follow up issue if that seems worthwhile to folks at this point in time.

Spec
image
Implementation
en-overflow
RTL
ar-overflow

Reviewer guidance

Points popover

  1. Log in as a learner
  2. Ensure you have completed some activities, so you have points
  3. Click on the placeholder icon - the popover with the correct number of points should display
  4. You should also be able to navigate to this by keyboard

Overflow

  1. Logged in as a learner OR as a coach: try out the display on different viewport sizes. While I imagine dynamic resizing the browser may be the less common reason this might pop up, that is a place to start. On hardware devices, or on the chrome dev tools that allow you to test on the viewport widths of mobile devices would be another.
  2. Navigation links should all work, either as a tab, or in the overflow, and direct the user to the correct page on navigation
  3. Works in RTL and LTR languages
  4. A11y specific: proper keyboard navigation, proper labeling/navigation for screen readers. @radinamatic much like above - I did do testing with voiceover and ensured there were aria labels on the buttons and 🤷‍♀️ I'm not sure if it is a good experience, or if it works across devices. Any suggestions for improvements welcome and appreciated

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

Sorry, something went wrong.

@marcellamaki marcellamaki added the work-in-progress Not ready for review label Aug 15, 2022
@marcellamaki marcellamaki added this to the 0.16.0 milestone Aug 15, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 15, 2022

@marcellamaki marcellamaki marked this pull request as ready for review August 16, 2022 20:08
@radinamatic
Copy link
Member

Tested with the DEB asset on Ubuntu 18.04, browsing as client from Firefox and Chrome on Windows 10 with NVDA.

Keyboard navigation and the screen reader experience is actually not bad: everything is accessible and announced, and all the users have similar experience - points display when the button is pressed, no mater which navigation modality is used.

points

pros/cons to not having the popover for screenreader users, and rather directly having the aria label just read aloud the points information

A part from the fact that actively trying to detect screen reader users is discouraged (and I'm not even sure there's a reliable way to do it), I don't think it's warranted here, since all users have the same experience (press-the-button-to-see-points).

The only issue I see here is that the popover remains open even when user navigates away: they have to actively press again for button to close, not even mouse click anywhere else does it 😛

Bigger concern in this new implementation is the insufficient contrast for the text in the subpage menu links: too low even for the currently active page, and way, way low for the rest 😞
cc @jtamiace

Ubuntu18 (start)  Running  - Oracle VM VirtualBox_010

@pcenov Could you test the non-accessibility pieces (mobile views & RTL) that @marcellamaki listed in the reviewer guidance? Thank you!

@pcenov
Copy link
Member

pcenov commented Aug 17, 2022

I was able to identify just the following couple of issues (valid in all supported browsers):

Issue Screenshot
Coach Plugin - The top navigation is not working except when a link is clicked through the 'More options'. There's a "TypeError: An id must be specified" error in the console https://user-images.githubusercontent.com/79847249/185111668-5a928721-7e9c-4961-9192-53fdf527878d.mp4
:-------------------------: :-------------------------:
Mobile view - The tooltip 'More options' is covering the text of the link (valid for RTL view too) 2022-08-17_14-44-01
:-------------------------: :-------------------------:

@marcellamaki marcellamaki force-pushed the points-popover-and-overflow branch from 617e567 to 509ed5d Compare August 18, 2022 15:58
@marcellamaki
Copy link
Member Author

Thank you @pcenov and @radinamatic - will follow up on these issues and update! 😊

@marcellamaki
Copy link
Member Author

marcellamaki commented Aug 23, 2022

@radinamatic @pcenov - I believe the contrast, coach navigation, popover "navigate away", and tooltip overlap should now all be addressed.

(Please note that in this PR, for the purposes of testing there is a temporary reference to an in-progress KDS work that supports this. If this is merged before that, the tooltip will still be overlapping until the KDS update is also merged)

@pcenov
Copy link
Member

pcenov commented Aug 24, 2022

@marcellamaki Looking great! Both the tooltip's overlapping of the menu items and coach navigation issues are now fixed. I just have a couple of questions:

Issue Screenshot
Facility > Classes > Class - Both tables in Enroll learners and Assign coaches are now stretching endlessly - is this a work in progress or an unintentional side effect? 2022-08-24_17-52-41
:-------------------------: :-------------------------:
Mobile view - Is the tooltip actually necessary in mobile as it is shown only after I've already tapped the button and seen what it does. Perhaps it should be removed altogether? 2022-08-24_17-47-42
:-------------------------: :-------------------------:

@radinamatic
Copy link
Member

Re-tested the newest asset, contrast is now within the guidelines! 👏🏽

I can replicate the finding by @pcenov about the Enroll learners table occupying the full width of the page. Is that the new design because the table now has 5 columns instead of 2?

I also second the observation that the tooltip does not seem necessary on touch interfaces. Anyways, it's not something that needs to be fixed in this PR, so going to approve 👍🏽

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual and a11y testing passed, good to go! 💯 :shipit:

Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @marcellamaki, overall this is great work. I didn't test it as I can see that the QA team has already tested it. I posted some clarifying questions and several cleanup comments. I'd welcome it if we could try to resolve the issue with the hidden dependency before merging. If we don't find a better way, then it'd be good to make sure to at least comment that the problematic class is being used from outside of NavbarLink.

kolibri/core/assets/src/views/AppBar.vue Outdated Show resolved Hide resolved
kolibri/core/assets/src/views/AppBar.vue Outdated Show resolved Hide resolved
kolibri/core/assets/src/views/Navbar/NavbarLink.vue Outdated Show resolved Hide resolved
kolibri/plugins/device/assets/src/views/DeviceTopNav.vue Outdated Show resolved Hide resolved
kolibri/plugins/device/assets/src/views/DeviceTopNav.vue Outdated Show resolved Hide resolved
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, as soon as the remaining media query in Navbar/index.vue is resolved, I think this is ready to be merged. If you found out that the media query needs to stay there, could you just please commit comment on why this was the case?

@MisRob
Copy link
Member

MisRob commented Sep 1, 2022

Oh, I realized we shouldn't actually merge this until we can update package.json, so just posting a note here so that we don't forget

@marcellamaki
Copy link
Member Author

marcellamaki commented Sep 2, 2022

Navbar/index.vue is resolved

Done! It was an oversight to not include with the other commits 🙃

@MisRob
Copy link
Member

MisRob commented Sep 8, 2022

I updated the PR with the following

  • Since we merged the corresponding KDS PR Update kdropdown menu api kolibri-design-system#346, I removed reference to Marcella's fork repo in favour of referencing the KDS repo in package.json
  • Resolved conflicts with the latest develop as of Sep 8 and did some related fixes of tests, see commit messages for more details
  • Resolved Property or method "disabled" is not defined on the instance but referenced during render. when the dropdown menu is rendered on smaller screens

MisRob
MisRob previously requested changes Sep 8, 2022
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When working on the above, I noticed that not all places using KDropdown are updated to its new API. At some places, we're still using text, disabled, and appearance props that have been removed from the corresponding KDS PR in learningequality/kolibri-design-system#346, for example

<KDropdownMenu
:text="$tr('optionsButtonLabel')"
:options="manageUserOptions(userRow.user.id)"
:disabled="!userCanBeEdited(userRow.user)"
appearance="flat-button"
@select="handleManageUserSelection($event, userRow.user)"
/>

We need to make sure to update KDropdown usage in the whole Kolibri before merging. In the example above, we need to wrap KDropdown component in a KDS button component and transfer text, disabled, and appearance properties there. There are more places like this.

I will see if I can get continue working on this next week, already know that I won't have time to continue this week, so blocking now.

@MisRob MisRob dismissed their stale review September 14, 2022 15:58

Resolved blocking issues

@MisRob
Copy link
Member

MisRob commented Sep 14, 2022

I pushed one more commit that updated our usage of KDropdownMenu, which resolved regressions at places we use dropdown menus. @radinamatic, if you could test all dropdown menus you can think of, that'd be helpful. These are places I located based on code:

dropdown-1 dropdown-2 dropdown-3 dropdown-4
dropdown-5 dropdown-6 dropdown-7 dropdown-8

Also, there's one more issue resulting in the console warning:

but this needs to be addressed in KDS. I will do it tomorrow.

@MisRob
Copy link
Member

MisRob commented Sep 14, 2022

I dismissed my review - it'd be good if someone else could review the last commit since I am the author. After that change is approved and @radinamatic confirms that this is good to go, should be ready for merge.

@MisRob
Copy link
Member

MisRob commented Sep 15, 2022

A couple of updates in KDS inspired by the latest work I made in this branch are here learningequality/kolibri-design-system#361. It also fixes 'Property or method "disabled" is not defined on the instance but referenced during render.'

It would be good to update package.json again here to point to the latest KDS develop as soon as learningequality/kolibri-design-system#361 is reviewed and merged.

MisRob and others added 27 commits September 23, 2022 08:56
… used in overflow menu, to prevent overlay. Requires corresponding KDS update
…ing updates. To be updated once KDS PRs updates
@marcellamaki marcellamaki force-pushed the points-popover-and-overflow branch from 8f840fd to 5b11d34 Compare September 23, 2022 12:56
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final issues resolved.

@rtibbles rtibbles merged commit 2bc72f7 into learningequality:develop Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants