Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. #5974

geetchoudhary · 2018-12-12T17:38:51Z

Explanation

Fixes #4936
The dropdown menus are now working similar to what the about and profile dropdown behaves with the keyboard. I.e on pressing

tab on the last element, the dropdown closes and the focus is passed on to the next element.
shift + tab on the first element, the dropdown closes and the focus is passed on to the previous element.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

codecov-io · 2018-12-12T18:26:54Z

Codecov Report

Merging #5974 into develop will decrease coverage by 0.02%.
The diff coverage is 2.56%.

@@             Coverage Diff             @@
##           develop    #5974      +/-   ##
===========================================
- Coverage     45.3%   45.27%   -0.03%     
===========================================
  Files          520      521       +1     
  Lines        30667    30688      +21     
  Branches      4597     4597              
===========================================
+ Hits         13893    13894       +1     
- Misses       16774    16794      +20

Impacted Files	Coverage Δ
...lates/dev/head/pages/library/SearchBarDirective.js	`1.02% <0%> (-0.1%)`	⬇️
...ts/top_navigation_bar/TopNavigationBarDirective.js	`0.88% <0%> (+0.07%)`	⬆️
...e/templates/dev/head/services/NavigationService.js	`4.16% <4.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92c4ecf...b10980e. Read the comment docs.

bansalnitish

Hi @geetchoudhary Thanks for the PR.
Did you test this locally? It's not working as expected. I went to the All Categories part and pressed tab on the last item, it moved to the next element but didn't closed the current drop down.

geetchoudhary · 2018-12-13T05:37:46Z

@bansalnitish yes, I just checked it again and it was working fine for me, are all the other things working as expected?

geetchoudhary · 2018-12-13T05:43:27Z

Sir, I am also attaching a video link in which I am testing the same.
Drive link

bansalnitish · 2018-12-13T08:22:49Z

Yes you're right @geetchoudhary, the working of these looks okay but did you check the working of About tab ? This is not working as expected.

geetchoudhary · 2018-12-13T08:29:09Z

Yes you're right @geetchoudhary, the working of these looks okay but did you check the working of About tab ? This is not working as expected.

@bansalnitish , There was nothing specified about the working of about tab in the issue #4936. Neither I have made any changes to it.

bansalnitish · 2018-12-13T08:56:10Z

Yeah, I know this PR doesn't includes anything related to the working of about tab. I think when the PR was made it did fix the about tab but again it's not working as expected.

Anyway, I'll review your PR within few hours. It's quite similar to PR.

geetchoudhary · 2018-12-13T08:58:15Z

Thanks :)

bansalnitish

I just tried and played with tab and shift-tab buttons on these two menus and the following are my observations:

Click on the All Categories button and then press tabs and go to the end and press tab. You'll notice this tab won't close.
Enter the All Categories menu by pressing entering after adjusting focus over it. You could do this by pressing tab once or twice right at the start. Focus will come to All Categories and then press tabs, reach the last element and then it will work fine. So if we enter by clicking the menu button it is not working fine.
This is minor but I noticed like in Profiles and the About tab if you press a tab focus goes to the first item of the menu whereas in All Categories and Language pressing the tab twice is required to shift the focus.

@geetchoudhary you could work on the first one as that is more important we can check for the third one later. Also, let me know if you too can reproduce the above three points, thanks!

geetchoudhary · 2018-12-13T19:26:24Z

@bansalnitish I have fixed the major issue. PTAL.

bansalnitish

It looks as if you pasted the stuff from the mentioned PR. You declared some functions but you haven't used them. Could you please correctly use these functions by referring to the above PR?

In general, it's good to refer to previous work but blind copying is not a good idea. Thanks!

core/templates/dev/head/pages/library/SearchBarDirective.js

bansalnitish

Left a comment for you. PTAL!

core/templates/dev/head/pages/library/SearchBarDirective.js

geetchoudhary · 2018-12-14T08:30:21Z

@bansalnitish Please check the changes.

bansalnitish

Thanks @geetchoudhary, LGTM!
@nithusha21 could you take a pass here ?

core/templates/dev/head/pages/library/SearchBarDirective.js

nithusha21 · 2018-12-14T17:38:19Z

I'll defer to @DubeySandeep :)

bansalnitish · 2018-12-16T17:28:53Z

@geetchoudhary any updates on this one ?

bansalnitish

I agree with @DubeySandeep, lets not repeat the code and instead have a service for the same.

(To avoid merge)

geetchoudhary · 2018-12-17T09:41:24Z

@geetchoudhary any updates on this one ?

@bansalnitish I tried working on it but could not get it to work. I am trying today also, will push the changes as soon as it is done.

bansalnitish · 2018-12-21T18:18:25Z

HI @geetchoudhary,
Any updates here ?

geetchoudhary · 2018-12-22T06:01:19Z

HI @geetchoudhary,
Any updates here ?

Hi @bansalnitish , I created a Service (NavigationService)

The service is working fine with SearchBarDirective.js with the following code

But the same service is not working with TopNavigationBarDirective.js

Can you please help me out?

bansalnitish · 2018-12-23T06:22:09Z

Hi @geetchoudhary, It's a bit hard to review the code through screenshots. Can you please push these changes?
Thanks

geetchoudhary · 2018-12-24T07:17:34Z

Hi @geetchoudhary, It's a bit hard to review the code through screenshots. Can you please push these changes?
Thanks

@bansalnitish PTAL.

bansalnitish

Hi @geetchoudhary, thanks for pushing the changes.

I think the comment below will resolve the issue. Also, could you please upload a clean code (without unnecessary comments) after which I'll take my next pass.

core/templates/dev/head/components/top_navigation_bar/TopNavigationBarDirective.js

bansalnitish

Hi @geetchoudhary, This is looking much better now, I have one more comment for you.

core/templates/dev/head/services/NavigationService.js

oppiabot · 2018-12-24T23:08:23Z

Hi @geetchoudhary. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

geetchoudhary · 2018-12-26T15:10:54Z

@bansalnitish . PTAL.

bansalnitish

Hi @geetchoudhary, this is really close now. Just one minor nit!

core/templates/dev/head/services/NavigationService.js

bansalnitish · 2018-12-26T18:07:13Z

Why do you need to merge develop into this PR again ? (Just a comment was needed to be addressed) Also, for what did you use force-push?

geetchoudhary · 2018-12-26T18:18:38Z

Why do you need to merge develop into this PR again ? (Just a comment was needed to be addressed) Also, for what did you use force-push?

@bansalnitish , because of the merge conflict in TopNavigationBar.js
$scope.currentUrl = window.location.pathname.split("/")[1];

I was getting this error

bansalnitish · 2018-12-26T18:20:45Z

You resolved the merge conflicts above, right ?

geetchoudhary · 2018-12-26T18:22:11Z

You resolved the merge conflicts above, right ?

Yes, everything is okay now.

bansalnitish · 2018-12-26T18:26:02Z

Still I don't think we really need to force-push. You could follow point 3 here - https://github.com/oppia/oppia/wiki/Git-cheat-sheet
Anyways, thanks!

bansalnitish

Thanks @geetchoudhary, good job :)
LGTM!

geetchoudhary · 2018-12-28T08:21:11Z

@bansalnitish , Kindly merge.

oppia#4936 adds tab and shift-tab navigation feature

67b9b8b

bansalnitish reviewed Dec 12, 2018

View reviewed changes

bansalnitish self-assigned this Dec 13, 2018

bansalnitish suggested changes Dec 13, 2018

View reviewed changes

section now closes when opened with mouseclick

48547d8

bansalnitish reviewed Dec 13, 2018

View reviewed changes

core/templates/dev/head/pages/library/SearchBarDirective.js Outdated Show resolved Hide resolved

core/templates/dev/head/pages/library/SearchBarDirective.js Outdated Show resolved Hide resolved

core/templates/dev/head/pages/library/SearchBarDirective.js Outdated Show resolved Hide resolved

Removed unwanted functions

fce8fa3

bansalnitish reviewed Dec 13, 2018

View reviewed changes

core/templates/dev/head/pages/library/SearchBarDirective.js Outdated Show resolved Hide resolved

Fixed function issues - added necessary functions

589b110

bansalnitish approved these changes Dec 14, 2018

View reviewed changes

DubeySandeep reviewed Dec 14, 2018

View reviewed changes

core/templates/dev/head/pages/library/SearchBarDirective.js Outdated Show resolved Hide resolved

core/templates/dev/head/pages/library/SearchBarDirective.js Outdated Show resolved Hide resolved

bansalnitish suggested changes Dec 16, 2018

View reviewed changes

geetchoudhary added 2 commits December 18, 2018 01:38

Added Service

3416322

Added service to top-navigation-bar

9fe23c1

Added NavigationService to aboutMenu

fd1cb8c

removed spaces

92fce29

geetchoudhary changed the title ~~Fix: 4936 The dropdown menus work similar to what the about and profile dropdown behave with the keyboard.~~ WIP : Fix: 4936 The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. Dec 24, 2018

bansalnitish reviewed Dec 24, 2018

View reviewed changes

core/templates/dev/head/components/top_navigation_bar/TopNavigationBarDirective.js Outdated Show resolved Hide resolved

geetchoudhary added 2 commits December 24, 2018 22:54

removed comments

4f7e88e

made rootScope accessible in html

54dd656

ankita240796 changed the title ~~WIP : Fix: 4936 The dropdown menus work similar to what the about and profile dropdown behave with the keyboard.~~ Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. Dec 24, 2018

bansalnitish reviewed Dec 24, 2018

View reviewed changes

core/templates/dev/head/services/NavigationService.js Outdated Show resolved Hide resolved

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Dec 24, 2018

geetchoudhary added 3 commits December 26, 2018 19:27

removed use of rootScope

62f866b

resolved merge conflict

dee8702

fixed pattern errors

fef273d

geetchoudhary changed the title ~~Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard.~~ WIP: Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. Dec 26, 2018

geetchoudhary changed the title ~~WIP: Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard.~~ Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. Dec 26, 2018

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Dec 26, 2018

bansalnitish reviewed Dec 26, 2018

View reviewed changes

core/templates/dev/head/services/NavigationService.js Outdated Show resolved Hide resolved

changed fileoverview

3fdf0e9

geetchoudhary force-pushed the tabnavigation branch from dc69a83 to 3fdf0e9 Compare December 26, 2018 18:02

Merge branch 'develop' into tabnavigation

b10980e

bansalnitish approved these changes Dec 26, 2018

View reviewed changes

bansalnitish merged commit 618344f into oppia:develop Dec 28, 2018

geetchoudhary deleted the tabnavigation branch December 30, 2018 04:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. #5974

Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. #5974

geetchoudhary commented Dec 12, 2018

codecov-io commented Dec 12, 2018 •

edited

Loading

bansalnitish left a comment

geetchoudhary commented Dec 13, 2018

geetchoudhary commented Dec 13, 2018 •

edited

Loading

bansalnitish commented Dec 13, 2018

geetchoudhary commented Dec 13, 2018 •

edited

Loading

bansalnitish commented Dec 13, 2018

geetchoudhary commented Dec 13, 2018

bansalnitish left a comment •

edited

Loading

geetchoudhary commented Dec 13, 2018

bansalnitish left a comment •

edited

Loading

bansalnitish left a comment

geetchoudhary commented Dec 14, 2018

bansalnitish left a comment

nithusha21 commented Dec 14, 2018

bansalnitish commented Dec 16, 2018

bansalnitish left a comment

geetchoudhary commented Dec 17, 2018

bansalnitish commented Dec 21, 2018

geetchoudhary commented Dec 22, 2018 •

edited

Loading

bansalnitish commented Dec 23, 2018 •

edited

Loading

geetchoudhary commented Dec 24, 2018

bansalnitish left a comment

bansalnitish left a comment

oppiabot bot commented Dec 24, 2018

geetchoudhary commented Dec 26, 2018

bansalnitish left a comment

bansalnitish commented Dec 26, 2018

geetchoudhary commented Dec 26, 2018

bansalnitish commented Dec 26, 2018

geetchoudhary commented Dec 26, 2018

bansalnitish commented Dec 26, 2018 •

edited

Loading

bansalnitish left a comment

geetchoudhary commented Dec 28, 2018 •

edited

Loading

Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. #5974

Fix #4936: The dropdown menus work similar to what the about and profile dropdown behave with the keyboard. #5974

Conversation

geetchoudhary commented Dec 12, 2018

Explanation

Checklist

codecov-io commented Dec 12, 2018 • edited Loading

Codecov Report

bansalnitish left a comment

Choose a reason for hiding this comment

geetchoudhary commented Dec 13, 2018

geetchoudhary commented Dec 13, 2018 • edited Loading

bansalnitish commented Dec 13, 2018

geetchoudhary commented Dec 13, 2018 • edited Loading

bansalnitish commented Dec 13, 2018

geetchoudhary commented Dec 13, 2018

bansalnitish left a comment • edited Loading

Choose a reason for hiding this comment

geetchoudhary commented Dec 13, 2018

bansalnitish left a comment • edited Loading

Choose a reason for hiding this comment

bansalnitish left a comment

Choose a reason for hiding this comment

geetchoudhary commented Dec 14, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

nithusha21 commented Dec 14, 2018

bansalnitish commented Dec 16, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

geetchoudhary commented Dec 17, 2018

bansalnitish commented Dec 21, 2018

geetchoudhary commented Dec 22, 2018 • edited Loading

bansalnitish commented Dec 23, 2018 • edited Loading

geetchoudhary commented Dec 24, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

bansalnitish left a comment

Choose a reason for hiding this comment

oppiabot bot commented Dec 24, 2018

geetchoudhary commented Dec 26, 2018

bansalnitish left a comment

Choose a reason for hiding this comment

bansalnitish commented Dec 26, 2018

geetchoudhary commented Dec 26, 2018

bansalnitish commented Dec 26, 2018

geetchoudhary commented Dec 26, 2018

bansalnitish commented Dec 26, 2018 • edited Loading

bansalnitish left a comment

Choose a reason for hiding this comment

geetchoudhary commented Dec 28, 2018 • edited Loading

codecov-io commented Dec 12, 2018 •

edited

Loading

geetchoudhary commented Dec 13, 2018 •

edited

Loading

geetchoudhary commented Dec 13, 2018 •

edited

Loading

bansalnitish left a comment •

edited

Loading

bansalnitish left a comment •

edited

Loading

geetchoudhary commented Dec 22, 2018 •

edited

Loading

bansalnitish commented Dec 23, 2018 •

edited

Loading

bansalnitish commented Dec 26, 2018 •

edited

Loading

geetchoudhary commented Dec 28, 2018 •

edited

Loading