Skip to content

Create AccessibleView, use it for terminal and chat accessibility help menus#184435

Merged
meganrogge merged 35 commits into
mainfrom
merogge/accessibleView
Jun 8, 2023
Merged

Create AccessibleView, use it for terminal and chat accessibility help menus#184435
meganrogge merged 35 commits into
mainfrom
merogge/accessibleView

Conversation

@meganrogge

@meganrogge meganrogge commented Jun 6, 2023

Copy link
Copy Markdown
Collaborator

@meganrogge meganrogge requested a review from Tyriar June 6, 2023 17:52
@meganrogge meganrogge marked this pull request as draft June 6, 2023 17:52
@meganrogge meganrogge self-assigned this Jun 6, 2023
@meganrogge meganrogge added this to the June 2023 milestone Jun 6, 2023
@meganrogge meganrogge marked this pull request as ready for review June 6, 2023 17:59

@rebornix rebornix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to leverage MultiCommand

export class MultiCommand extends Command {
so it's a single command for Show Accessibility Help View in all places, which can give us benefits of:

  • Single command contribution, each contributor only needs to register a provider other than both the provider and the action contribution
  • If users want to change the keybinding, they don't have to change the keybindings for every *.showAccessibilityHelpView command.
  • This command can then also be added to the Menu

Current approach doesn't seem to scale.

@meganrogge

Copy link
Copy Markdown
Collaborator Author

Yes @rebornix i thought the same and was unaware such a thing existed. Thanks!

@meganrogge

meganrogge commented Jun 6, 2023

Copy link
Copy Markdown
Collaborator Author

@rebornix i'll need to defer that to the PR where I convert the editor accessibility help action to use this - currently, what's happening is that is overriding the terminal's action since there's no precondition. I'll need to add a when clause to the addImplementation arguments.

Edit:

got it to work

@meganrogge meganrogge enabled auto-merge June 6, 2023 20:37
Tyriar
Tyriar previously requested changes Jun 6, 2023
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
@meganrogge meganrogge requested a review from Tyriar June 7, 2023 17:44
Comment thread src/vs/editor/standalone/browser/accessibilityHelp/accessibilityHelp.css Outdated
@meganrogge meganrogge disabled auto-merge June 7, 2023 18:13
Tyriar
Tyriar previously requested changes Jun 7, 2023
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
Comment thread src/vs/workbench/contrib/accessibility/browser/accessibleView.ts Outdated
@meganrogge meganrogge requested a review from Tyriar June 7, 2023 19:47
@meganrogge meganrogge enabled auto-merge June 7, 2023 20:09
Tyriar
Tyriar previously approved these changes Jun 8, 2023
@meganrogge meganrogge merged commit f2c5300 into main Jun 8, 2023
@meganrogge meganrogge deleted the merogge/accessibleView branch June 8, 2023 15:51
@meganrogge meganrogge restored the merogge/accessibleView branch June 8, 2023 20:33
cadinsl pushed a commit to cadinsl/vscode that referenced this pull request Jun 13, 2023
…View

Create `AccessibleView`, use it for terminal and chat accessibility help menus
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 23, 2023
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.

Make accessibility help generic make accessibility help menu more accessible

4 participants