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 navigation, modality, and confirm #70937

Merged
merged 6 commits into from Mar 22, 2019
Merged

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Mar 22, 2019

No description provided.

@sbatten sbatten requested a review from bpasero March 22, 2019 06:25
@sbatten sbatten self-assigned this Mar 22, 2019
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@sbatten good stuff overall. I added one request for change.

Then some issues I noticed:

  • looks like mnemonic handling is still needed (see image below)
  • while a dialog is visible, the dialog service should probably not allow for another dialog to open (e.g. have a queue of dialogs to show when multiple are requested)
  • colors: I see the changes in theme, but see no explicit color definitions for extensions to tweak? do we need new colors for the dialog?
  • colors: I see you are using Button widget, but I see no updating of its styles when themes change. I think you need to propagate that including the button colors. we have other widgets as examples that are composed of multiple themeable things in base
  • the close button does not seem to be accessible via keyboard navigation. Update: actually do not change this, its good!
  • ARIA: we might need to explicitly announce the dialog when it opens, there is maybe examples for how to do custom dialogs in terms of accessibility
  • I am still able to bring up quick open (Cmd+P) while a modal dialog is open, which should probably be prevented

image

src/vs/platform/theme/common/styler.ts Outdated Show resolved Hide resolved
@bpasero bpasero added this to the March 2019 milestone Mar 22, 2019
@bpasero
Copy link
Member

bpasero commented Mar 22, 2019

@misolori I see we are probably just using our existing error/warning/info icons in the dialogs, but I feel like they do not fit as good as in the small version. Any thoughts on changing them for the dialogs?

image

@sbatten sbatten merged commit f832022 into microsoft:master Mar 22, 2019
@sbatten sbatten deleted the dialogService branch March 22, 2019 18:46
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants