-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(webview): persistent NEW badge over version indicator for new announcements #7288
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
Conversation
…-session acknowledgement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own code and found it surprisingly not terrible, which is suspicious.
| aria-label={t("chat:versionIndicator.ariaLabel", { version: Package.version })}> | ||
| v{Package.version} | ||
| {showBadge && ( | ||
| <span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing aria-label for the badge. Screen reader users won't know what "NEW" refers to. Consider adding:
| <span | |
| <span | |
| // The badge uses VS Code theme variables to blend with light/dark themes | |
| aria-label="New announcement available" | |
| className="absolute -top-1.5 -right-1.5 rounded-full bg-vscode-button-background text-vscode-button-foreground text-[10px] leading-none px-1.5 py-0.5 shadow ring-2 ring-vscode-editor-background select-none"> |
| useEffect(() => { | ||
| // Only update if the extension indicates we should show the badge | ||
| // This allows local state to clear immediately after user clicks | ||
| if (shouldShowAnnouncementBadge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The badge state syncs from extension state but could be overridden by local state changes. The comment suggests it's intentional to allow immediate local clearing, but this could create a race condition if the extension state updates at the same time. Consider using a more explicit state management pattern or documenting this behavior more clearly.
| <span | ||
| // The badge uses VS Code theme variables to blend with light/dark themes | ||
| className="absolute -top-1.5 -right-1.5 rounded-full bg-vscode-button-background text-vscode-button-foreground text-[10px] leading-none px-1.5 py-0.5 shadow ring-2 ring-vscode-editor-background select-none"> | ||
| NEW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "NEW" text is hardcoded. Could this be made configurable or use i18n for localization? Something like t('common:badge.new') would make it more flexible for international users.
| {showBadge && ( | ||
| <span | ||
| // The badge uses VS Code theme variables to blend with light/dark themes | ||
| className="absolute -top-1.5 -right-1.5 rounded-full bg-vscode-button-background text-vscode-button-foreground text-[10px] leading-none px-1.5 py-0.5 shadow ring-2 ring-vscode-editor-background select-none"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting these badge styles to a reusable component or at least a constant. This long className string could be:
const BADGE_STYLES = "absolute -top-1.5 -right-1.5 rounded-full bg-vscode-button-background text-vscode-button-foreground text-[10px] leading-none px-1.5 py-0.5 shadow ring-2 ring-vscode-editor-background select-none"This would improve maintainability and reusability.
|
@roomote-agent The little "new" is handy but the placement of the version indicator seems to have gotten buggered up |
|
Hi @hannesrudolph, I see the issue with the version indicator placement. Let me investigate and fix this for you. |
- Moved VersionIndicator to be a direct child of the main welcome screen container - This ensures proper absolute positioning relative to the entire welcome area - Fixes layout issue where the version indicator was misaligned due to nested container positioning
|
Hi @hannesrudolph, I've fixed the version indicator placement issue! The problem was that the VersionIndicator was positioned inside a nested container with its own padding and layout constraints. I moved it to be a direct child of the main welcome screen container (the div with Changes made:
The fix has been pushed to the PR branch and all tests are passing. CI checks are currently running and looking good so far. The version indicator should now be properly positioned in the top-right corner of the welcome screen. |
Implements a persistent "NEW" badge over the version number on the welcome/hero screen when a new announcement triggers. Clicking the version opens the announcement modal and clears the badge. The badge persists across reloads until acknowledged.
Key changes:
Tests:
Important
Adds a persistent "NEW" badge over the version indicator for new announcements, which persists across sessions until acknowledged by the user.
VersionIndicatorwhen a new announcement is available.VersionIndicatorupdated to show "NEW" badge whenshowBadgeis true.ChatViewandApphandle badge visibility and click events to update state and notify the extension.lastAcknowledgedAnnouncementIdinglobal-settings.tsto track badge state.ClineProviderandwebviewMessageHandlerto manage cross-session badge persistence.This description was created by
for e402468. You can customize this summary. It will automatically update as commits are pushed.