Enhancement: Make filter section collapsible to improve popup layout …#388
Enhancement: Make filter section collapsible to improve popup layout …#388ugupta17k wants to merge 1 commit intofossasia:mainfrom
Conversation
Reviewer's GuideIntroduces a collapsible "Advanced Filters" section in the popup UI, wiring it to persistent toggle behavior via chrome.storage and documenting the new behavior in the README and locale strings. Sequence diagram for collapsible advanced filters state persistencesequenceDiagram
actor User
participant PopupUI
participant PopupJS
participant ChromeStorage
%% Initial load
User->>PopupUI: Open extension popup
PopupUI->>PopupJS: DOMContentLoaded
PopupJS->>ChromeStorage: get advancedFiltersOpen
ChromeStorage-->>PopupJS: advancedFiltersOpen value
alt advancedFiltersOpen is true
PopupJS->>PopupUI: Remove hidden from advancedFiltersContainer
PopupJS->>PopupUI: Replace fa-chevron-down with fa-chevron-up on advancedFiltersIcon
else advancedFiltersOpen is false or undefined
PopupJS->>PopupUI: Ensure advancedFiltersContainer has hidden
PopupJS->>PopupUI: Ensure advancedFiltersIcon has fa-chevron-down
end
%% User toggles advanced filters
User->>PopupUI: Click advancedFiltersToggle button
PopupUI->>PopupJS: advancedFiltersToggle click event
PopupJS->>PopupJS: Check advancedFiltersContainer.hidden
alt Container is open
PopupJS->>PopupUI: Add hidden to advancedFiltersContainer
PopupJS->>PopupUI: Replace fa-chevron-up with fa-chevron-down on advancedFiltersIcon
PopupJS->>ChromeStorage: set advancedFiltersOpen = false
else Container is closed
PopupJS->>PopupUI: Remove hidden from advancedFiltersContainer
PopupJS->>PopupUI: Replace fa-chevron-down with fa-chevron-up on advancedFiltersIcon
PopupJS->>ChromeStorage: set advancedFiltersOpen = true
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
popup.js,advancedFiltersContainerandadvancedFiltersIconare used but onlyadvancedFiltersToggleis added to theidsToGetlist, so make sure you’re actually querying and assigning those elements before using them to avoid runtime errors. - When restoring the advanced filters state from
chrome.storage, consider explicitly setting both the container visibility and the chevron icon class for both open and closed cases so the UI is always in a known, consistent state (including when no value is stored yet). - For the new
Advanced Filtersbutton, adding appropriate ARIA attributes (e.g.aria-expanded,aria-controls) and updating them on toggle would improve accessibility of the collapsible section.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `popup.js`, `advancedFiltersContainer` and `advancedFiltersIcon` are used but only `advancedFiltersToggle` is added to the `idsToGet` list, so make sure you’re actually querying and assigning those elements before using them to avoid runtime errors.
- When restoring the advanced filters state from `chrome.storage`, consider explicitly setting both the container visibility and the chevron icon class for both open and closed cases so the UI is always in a known, consistent state (including when no value is stored yet).
- For the new `Advanced Filters` button, adding appropriate ARIA attributes (e.g. `aria-expanded`, `aria-controls`) and updating them on toggle would improve accessibility of the collapsible section.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hi @ugpta17k, I’ve already implemented this enhancement and opened PR #387 earlier, which is linked above in the thread and is currently under review. The functionality you described is already covered there. Since the work is already in progress, please consider closing PR #388 to avoid duplication. @hpdang @vedansh-5 — could you please take a look and close the duplicate PR if appropriate? Thank you. |
|
Thanks @ugupta17k for your contribution. However #387 addresses this issue. Therefore closing this PR. |
…(#385)
📌 Fixes
Fixes # (Use "Fixes", "Closes", or "Resolves" for automatic closing)
📝 Summary of Changes
📸 Screenshots / Demo (if UI-related)
Add screenshots, video, or link to deployed preview if applicable
✅ Checklist
👀 Reviewer Notes
Add any special notes for the reviewer here
Summary by Sourcery
Make the popup’s filter controls collapsible under an Advanced Filters section to keep the UI compact while preserving existing options.
Enhancements:
Documentation: