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

Editor Menubar Example: Improve high contrast support and refactor Javascript #1356

Merged
merged 77 commits into from
Jul 13, 2020

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Mar 19, 2020

closes #1355

This pull request is to update the editor menubar example.

Editor Menubar Preview link

The goal of the update is to:

  1. Simplify the Javascript code and use a single object
  2. Improve high contrast support
  3. The Javascript APIs will make it easy to use with ARIA AT setup scripts

Review checklist

@jongund
Copy link
Contributor Author

jongund commented Mar 19, 2020

@spectranaut

There is a problem with the regression test "key.SPACE" feature, it does not seem to be generating the correct code for testing. I had to add some code on line 524 to pass the regression test for the
"space" character test. Can you look into this issue?

@jongund jongund requested review from spectranaut and zcorpan March 19, 2020 19:39
@jongund jongund changed the title Issue1355 update menubar editor js ssue1355 update menubar editor javascript and high contrast compatibility Mar 25, 2020
@jongund jongund changed the title ssue1355 update menubar editor javascript and high contrast compatibility update menubar editor javascript and high contrast compatibility Mar 26, 2020
@jongund jongund linked an issue Mar 26, 2020 that may be closed by this pull request
@a11ydoer a11ydoer self-requested a review March 31, 2020 18:23
@smhigley
Copy link
Contributor

smhigley commented Apr 1, 2020

@jongund I pushed a commit that fixes the failing spacebar test, based on my comment in #1358. All the tests pass locally for me now :)

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Completed editorial review. I pushed commits to fix some minor issues. All the copy looks good. Thank you @jongund.

@mcking65 mcking65 removed the request for review from spectranaut April 14, 2020 05:30
@shirsha
Copy link

shirsha commented May 8, 2020

  • In the updated menubar, the menu's that have sub-menus have no visual indicator as such.

  • The menu closes whenever user selects. They must open it each time to make more selections. Ex: Style/color. The user can select more than one style from the available options but the menu closes as soon as user selects any one option. Similarly, when user select "Smaller" in Size by default "medium” font size is selected. To change the font size, the user must open the Size menu and change the selection.

  • The red color text (16px) against white background doesn't pass the required color contrast ratio of 4.5:1

  • In Andriod, menus are not consistently expanding on touch.If a menu is open, it takes two touches to open another menu. It is working fine in iOS.

@jongund
Copy link
Contributor Author

jongund commented Jul 8, 2020

@mcking65
I fixed the merge conflicts, but the regression test file test\tests\menubar_menubar-2.js still needs to be removed, the example it once tested has been changed to menubar_menubar-editor,js. I tired to remove the file, but it generates a merge conflict, so I put it back in. Maybe you can fix it.

@jongund
Copy link
Contributor Author

jongund commented Jul 8, 2020

@mcking65
As I thought the regression test test\tests\menubar_menubar-2.js is failing, because the example it references does not exist anymore (e.g. new example name menubar-editor.html and new test file test\tests\menubar_menubar-editor.js). When I delete the uneeded test file from the branch it gives the merge conflict in GitHub you reported earlier, so I am not sure what to do fix the problem. Seems like a catch 22 for me.

@mcking65
Copy link
Contributor

mcking65 commented Jul 8, 2020

@jongund, what if we keep the current editor menubar and its tests; just make this PR about adding new versions with new names/location. Then, after we merge this, we do a separate PR to remove the unnecessary files? I wonder if that would solve the problem.

It doesn't seem like such gymnasitcs should be necessary ... I guess this calls for further investigationit is

@jongund
Copy link
Contributor Author

jongund commented Jul 9, 2020

@mcking65
Restored original menubar editor example files, seems like that should fix the merge conflict problem.

@jongund
Copy link
Contributor Author

jongund commented Jul 9, 2020

@mcking65
Looks like it is ready to merge now.

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

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

This all looks great to me! Very readable and simple code, perfect for an example. And the tests all apply to this example and run successfully. I fixed one spelling error in a comment.

@mcking65 mcking65 merged commit c9524a5 into master Jul 13, 2020
@mcking65 mcking65 deleted the issue1355-update-menubar-editor-js branch July 13, 2020 19:06
michael-n-cooper pushed a commit that referenced this pull request Jul 13, 2020
Editor Menubar Example: Improve high contrast support and refactor Javascript (pull #1356)

Resolves issue #1355 for the editor menubar example by:
* Simplifying the Javascript code to use a single object.
* Replace PNG files with SVG.
* Improving high contrast support
* Makes corresponding editorial revisions to documentation.
* Fixes bug with testing space key.

Co-authored-by: Sarah Higley <sarah.higley@microsoft.com>
Co-authored-by: Matt King <a11yThinker@gmail.com>
Co-authored-by: Valerie Young <valerie@bocoup.com>
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.

Update menubar editor example Javascript
10 participants