Conversation
Screenshot ChangesBase: Changed (2)
|
| Before | After |
|---|---|
 |
 |
editor/inlineCompletions/other/JumpToHint/Dark
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Welcome Onboarding “Variation A” modal UI by replacing the footer “Skip” action with a close icon button in the card’s upper-right corner, along with the necessary layout and focus adjustments.
Changes:
- Add an icon-only close button to the onboarding card and wire it to the existing dismiss/skip behavior.
- Remove the footer “Skip” button and associated references.
- Update styling to support positioning and visuals for the new close control.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/contrib/welcomeOnboarding/browser/onboardingVariationA.ts |
Adds the close button, removes skip button wiring, and updates focus fallback logic. |
src/vs/workbench/contrib/welcomeOnboarding/browser/media/variationA.css |
Makes the card a positioning context and introduces styles for the new close button. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
| private _getFocusableElements(): HTMLElement[] { | ||
| return [...this.stepFocusableElements, ...this.footerFocusableElements].filter(element => this._isTabbable(element)); | ||
| } | ||
|
|
||
| private _focusCurrentStepElement(): void { | ||
| const stepFocusable = this.stepFocusableElements.find(element => this._isTabbable(element)); | ||
| (stepFocusable ?? this.nextButton ?? this.skipButton)?.focus(); | ||
| (stepFocusable ?? this.nextButton ?? this.closeButton)?.focus(); | ||
| } |
There was a problem hiding this comment.
The focus-trap order no longer matches the DOM/tab order now that the close button is a tabbable element placed before the step content. _getFocusableElements() returns step elements before closeButton, so Shift+Tab on the first step control wraps to the last control (skipping the close button), and Shift+Tab on the close button can escape the dialog entirely. Consider including closeButton as the first element in the focusables list (separate from footerFocusableElements), so the trap’s first/last align with the actual tab sequence within the dialog.
| // Close button (upper-right corner of card) | ||
| this.closeButton = append(this.card, $<HTMLButtonElement>('button.onboarding-a-close-btn')); | ||
| this.closeButton.type = 'button'; | ||
| this.closeButton.setAttribute('aria-label', localize('onboarding.close', "Close")); |
There was a problem hiding this comment.
The close button is icon-only; it has an aria-label, but it doesn’t set a title tooltip. Elsewhere we typically set a localized title on close/dismiss affordances to provide a hover tooltip and improve discoverability for mouse users.
| this.closeButton.setAttribute('aria-label', localize('onboarding.close', "Close")); | |
| const closeButtonLabel = localize('onboarding.close', "Close"); | |
| this.closeButton.setAttribute('aria-label', closeButtonLabel); | |
| this.closeButton.title = closeButtonLabel; |
Co-authored-by: Copilot <copilot@github.com>
Changes "Skip" button to close the walkthrough to a
X. Reduce confusion on behavior of Skip vs Next actionsUI Changes:
.onboarding-a-close-btnclose button to the upper-right corner of the onboarding card, with appropriate styling and accessibility features. [1] [2]Behavior and Accessibility:
Layout Adjustments:
position: relativeto properly position the new close button.