🐛 Filter button: icon-only with glow indicator#4370
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
bot
added
the
dco-signoff: yes
✅ Deploy Preview for kubestellarconsole canceled.
|
kubestellar-prow
bot
added
the
needs-rebase
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/XXL
There was a problem hiding this comment.
Pull request overview
This PR is a UI follow-up to #4369 that removes text from the navbar filter controls to prevent layout shift, while also consolidating the prior “Projects” concept into the unified filter dropdown (via cluster groups) and migrating legacy localStorage data.
Changes:
- Converted the navbar filter trigger and clear action to icon-only buttons, adding a visual “active” glow/dot indicator.
- Added a “Projects” (cluster groups) section inside the unified filter dropdown, including create/delete and color selection.
- Removed the legacy project filter implementation (context + selector + definitions) and added a one-time localStorage migration into
clusterGroups.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/lib/projects.ts | Removed legacy project definitions/constants (projects feature retired). |
| web/src/hooks/useProjectFilter.tsx | Removed legacy project filter context/provider and CRUD logic. |
| web/src/hooks/useGlobalFilters.tsx | Added migration from legacy projects storage to cluster groups. |
| web/src/components/layout/navbar/ProjectSelector.tsx | Removed legacy project selector dropdown from navbar. |
| web/src/components/layout/navbar/Navbar.tsx | Switched navbar to use only the unified ClusterFilterPanel. |
| web/src/components/layout/navbar/ClusterFilterPanel.tsx | Implemented icon-only filter/clear buttons + “Projects” section UI and group color support. |
| web/src/App.tsx | Removed ProjectFilterProvider from the app provider tree. |
| // Initialize cluster groups from localStorage | ||
| // Initialize cluster groups from localStorage (+ migrate legacy projects) | ||
| const [clusterGroups, setClusterGroups] = useState<ClusterGroup[]>(() => { | ||
| let groups: ClusterGroup[] = DEFAULT_GROUPS |
There was a problem hiding this comment.
In the cluster-group initializer, groups is assigned to DEFAULT_GROUPS and then later mutated via groups.push(...). This mutates the DEFAULT_GROUPS constant array by reference (even though it’s currently empty), which can cause surprising shared-state behavior. Initialize with a new array (e.g., spread/copy) before mutating.
| let groups: ClusterGroup[] = DEFAULT_GROUPS | |
| let groups: ClusterGroup[] = [...DEFAULT_GROUPS] |
| const existingNames = new Set(groups.map(g => g.name)) | ||
| for (const p of projects) { | ||
| if (!existingNames.has(p.name)) { | ||
| groups.push({ | ||
| id: `migrated-${p.id}`, |
There was a problem hiding this comment.
During legacy projects → cluster groups migration, existingNames is never updated after adding a migrated group. If multiple legacy projects share the same name, this loop can insert duplicates in a single run. Add the name to existingNames when a group is appended (or otherwise dedupe).
| const projects = JSON.parse(oldProjects) as Array<{ | ||
| id: string; name: string; clusters: string[]; color?: string | ||
| }> | ||
| if (Array.isArray(projects) && projects.length > 0) { | ||
| const existingNames = new Set(groups.map(g => g.name)) | ||
| for (const p of projects) { | ||
| if (!existingNames.has(p.name)) { | ||
| groups.push({ | ||
| id: `migrated-${p.id}`, | ||
| name: p.name, | ||
| clusters: p.clusters || [], | ||
| color: p.color, | ||
| }) | ||
| } | ||
| } | ||
| localStorage.setItem(GROUPS_STORAGE_KEY, JSON.stringify(groups)) | ||
| } | ||
| localStorage.removeItem('projects:definitions') | ||
| localStorage.removeItem('projects:selected') |
There was a problem hiding this comment.
The migration removes projects:definitions / projects:selected whenever the key exists and JSON.parse succeeds, even if the parsed value isn’t a valid projects array or nothing was migrated. This can permanently drop user data. Only remove the legacy keys after validating the parsed structure and successfully migrating.
| const projects = JSON.parse(oldProjects) as Array<{ | |
| id: string; name: string; clusters: string[]; color?: string | |
| }> | |
| if (Array.isArray(projects) && projects.length > 0) { | |
| const existingNames = new Set(groups.map(g => g.name)) | |
| for (const p of projects) { | |
| if (!existingNames.has(p.name)) { | |
| groups.push({ | |
| id: `migrated-${p.id}`, | |
| name: p.name, | |
| clusters: p.clusters || [], | |
| color: p.color, | |
| }) | |
| } | |
| } | |
| localStorage.setItem(GROUPS_STORAGE_KEY, JSON.stringify(groups)) | |
| } | |
| localStorage.removeItem('projects:definitions') | |
| localStorage.removeItem('projects:selected') | |
| const parsedProjects: unknown = JSON.parse(oldProjects) | |
| const isValidProject = ( | |
| project: unknown | |
| ): project is { id: string; name: string; clusters: string[]; color?: string } => { | |
| if (!project || typeof project !== 'object') { | |
| return false | |
| } | |
| const candidate = project as { | |
| id?: unknown; name?: unknown; clusters?: unknown; color?: unknown | |
| } | |
| return ( | |
| typeof candidate.id === 'string' && | |
| typeof candidate.name === 'string' && | |
| Array.isArray(candidate.clusters) && | |
| candidate.clusters.every(cluster => typeof cluster === 'string') && | |
| (candidate.color === undefined || typeof candidate.color === 'string') | |
| ) | |
| } | |
| if (Array.isArray(parsedProjects) && parsedProjects.length > 0 && parsedProjects.every(isValidProject)) { | |
| const existingNames = new Set(groups.map(g => g.name)) | |
| const migratedGroups: ClusterGroup[] = [] | |
| for (const p of parsedProjects) { | |
| if (!existingNames.has(p.name)) { | |
| migratedGroups.push({ | |
| id: `migrated-${p.id}`, | |
| name: p.name, | |
| clusters: p.clusters, | |
| color: p.color, | |
| }) | |
| existingNames.add(p.name) | |
| } | |
| } | |
| if (migratedGroups.length > 0) { | |
| const nextGroups = [...groups, ...migratedGroups] | |
| localStorage.setItem(GROUPS_STORAGE_KEY, JSON.stringify(nextGroups)) | |
| groups = nextGroups | |
| localStorage.removeItem('projects:definitions') | |
| localStorage.removeItem('projects:selected') | |
| } | |
| } |
| <button | ||
| onClick={() => { | ||
| selectAllClusters() | ||
| selectAllSeverities() | ||
| selectAllStatuses() |
There was a problem hiding this comment.
The clear-filters button is now icon-only and relies on title for its description. title is not a reliable accessible name for screen readers. Add an aria-label (and consider type="button") so the control is accessible.
| <button | ||
| onClick={() => setShowClusterFilter(!showClusterFilter)} | ||
| className={cn( | ||
| 'flex items-center gap-2 px-3 py-1.5 rounded-lg transition-colors', | ||
| 'relative flex items-center justify-center w-8 h-8 rounded-lg transition-colors', | ||
| isFiltered |
There was a problem hiding this comment.
The filter toggle button is icon-only and relies on title. Please add an aria-label and expose state via aria-pressed={isFiltered} (or similar) so assistive tech can understand and operate the control.
| </button> | ||
| <button | ||
| onClick={() => deleteClusterGroup(group.id)} | ||
| className="p-1 text-muted-foreground hover:text-red-400 opacity-0 group-hover/item:opacity-100 transition-all flex-shrink-0" |
There was a problem hiding this comment.
The cluster-group delete button (trash icon) relies on title and has no accessible name for screen readers. Add an aria-label so the action is announced correctly.
| className="p-1 text-muted-foreground hover:text-red-400 opacity-0 group-hover/item:opacity-100 transition-all flex-shrink-0" | |
| className="p-1 text-muted-foreground hover:text-red-400 opacity-0 group-hover/item:opacity-100 transition-all flex-shrink-0" | |
| aria-label={t('common:filters.deleteGroup', 'Delete group')} |
| {GROUP_COLORS.map(c => ( | ||
| <button | ||
| key={c} | ||
| onClick={() => setNewGroupColor(c)} | ||
| className={cn( |
There was a problem hiding this comment.
The color picker swatch buttons are purely visual and currently have no accessible name/state. Add an aria-label per color and expose selection via aria-pressed (or an equivalent pattern) so the picker is usable with assistive tech.
Remove "All"/"Filtered" text label from filter button to prevent navbar layout shift. Now shows just the funnel icon with: - Purple background + glow when filters are active - Small purple dot indicator in top-right corner - Compact X button to clear filters (also icon-only) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
clubanderson
force-pushed
the
feat/merge-project-filter
branch
from
22b4a61 to
93f82ce
Compare
kubestellar-prow
bot
added
size/M
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
✅ Post-Merge Verification: passedCommit: |
Summary
Follow-up to #4369 — removes text from the filter button to prevent navbar layout shift.
Before: Button showed "All" → "Filtered" → filter set name (text changes caused elements to jump)
After: Icon-only button with visual state indicators:
Test plan