-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,17 +5,26 @@ import { Package } from "@roo/package" | |||||||||||
| interface VersionIndicatorProps { | ||||||||||||
| onClick: () => void | ||||||||||||
| className?: string | ||||||||||||
| // When true, renders a small "NEW" badge over the version button | ||||||||||||
| showBadge?: boolean | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const VersionIndicator: React.FC<VersionIndicatorProps> = ({ onClick, className = "" }) => { | ||||||||||||
| const VersionIndicator: React.FC<VersionIndicatorProps> = ({ onClick, className = "", showBadge = false }) => { | ||||||||||||
| const { t } = useTranslation() | ||||||||||||
|
|
||||||||||||
| return ( | ||||||||||||
| <button | ||||||||||||
| onClick={onClick} | ||||||||||||
| className={`text-xs text-vscode-descriptionForeground hover:text-vscode-foreground transition-colors cursor-pointer px-2 py-1 rounded border ${className}`} | ||||||||||||
| className={`relative inline-flex items-center text-xs text-vscode-descriptionForeground hover:text-vscode-foreground transition-colors cursor-pointer px-2 py-1 rounded border ${className}`} | ||||||||||||
| aria-label={t("chat:versionIndicator.ariaLabel", { version: Package.version })}> | ||||||||||||
| v{Package.version} | ||||||||||||
| {showBadge && ( | ||||||||||||
| <span | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Suggested change
|
||||||||||||
| // 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"> | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||
| NEW | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||
| </span> | ||||||||||||
| )} | ||||||||||||
| </button> | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
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.