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

Fix: Add tooltips and correct color contrast #619

Merged
merged 8 commits into from
Jul 1, 2020
Merged

Conversation

ElinorW
Copy link
Collaborator

@ElinorW ElinorW commented Jun 26, 2020

Overview

  • Added tooltips to the 'minimize sidebar' and 'more actions' icons displayed to enhance keyboard navigation.

  • Changed the color of the "patch" drop-down control to fit the color contrast ratio.

Demo

image

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Testing Instructions

  • How to test this PR
  • Prefer bulleted description
  • Start after checking out this branch
  • Include any setup required, such as bundling scripts, restarting services, etc.
  • Include test case, and expected output

@ElinorW ElinorW requested review from ddyett and thewahome June 26, 2020 13:11
@ElinorW ElinorW self-assigned this Jun 26, 2020
@jobala
Copy link
Contributor

jobala commented Jun 26, 2020

I see two "more actions" tooltips when hovering over the gear icon.

image

@ElinorW ElinorW requested a review from jobala June 26, 2020 16:01
ariaLabel='Minimise sidebar'
onClick={() => toggleSidebar()} />
<TooltipHost
content='Minimise sidebar'
Copy link

Choose a reason for hiding this comment

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

misspelt. should be z

Copy link

Choose a reason for hiding this comment

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

also what about localization here?

Copy link
Contributor

Choose a reason for hiding this comment

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

misspelt. should be z

@ddyett depends on which english pronunciation you're using 👽

Copy link

Choose a reason for hiding this comment

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

all the more reason for localization. since the most popular page is en-US probably should be z.

@jobala
Copy link
Contributor

jobala commented Jun 26, 2020

@ddyett @bettirosengugi @ElinorW @thewahome

I am interested in knowing what you guys think about replacing the settings icon with a context menu icon. The settings icon is used for updating user settings or profile in apps. The context menu icon is used for showing more actions.

Sample Context Menu Icon
image

@ddyett
Copy link

ddyett commented Jun 26, 2020

not sure how i feel about the icon. there are some settings like actions and some more actions like actions. i'd have to think about it.

@jobala
Copy link
Contributor

jobala commented Jun 26, 2020

@ddyett these are a closer representation of how the context-menu icons look like in uifabric-react
image

@Shjokie
Copy link
Contributor

Shjokie commented Jul 1, 2020

@ddyett these are a closer representation of how the context-menu icons look like in uifabric-react
image

This is a good call out @jobala We got this feedback from Frederick as well. @ddyett may be we can have this as our first case for A/B testing?

@ElinorW ElinorW merged commit 958055a into dev Jul 1, 2020
@thewahome thewahome added this to the Next release candidate milestone Jul 20, 2020
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.

None yet

5 participants