feat: add inline variant to Filters component (#1) #365
Conversation
|
@Copilot is attempting to deploy a commit to the Hycom Team Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FiltersSection
participant Filters
participant FormikForm
User->>FiltersSection: Render with variant='inline'
FiltersSection->>Filters: forward variant
alt variant == 'inline'
Filters->>FormikForm: mount inline Formik form
FormikForm->>Filters: provide form state & handlers
Filters->>User: render inline items and action buttons
else variant == 'drawer'
Filters->>User: render drawer UI (existing flow)
end
User->>Filters: interact (set/reset/submit)
Filters->>FormikForm: update/submit form
FormikForm->>User: reflect updated state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/src/components/Filters/Filters.tsx (1)
60-101: Leading items are not rendered in the inline variant.The inline variant only renders
filteredItems(line 77), completely ignoring any leading items that might be present. While this may be intentional, it creates inconsistent behavior between variants and could be unexpected:
- The
hasLeadingItemprop is accepted but not used in the inline variant- Users expecting leading items to appear will see them in drawer mode but not in inline mode
- The drawer variant has special handling for leading items (lines 118-130 in the same file)
Consider one of these approaches:
- Document this behavior in the component's JSDoc or README
- Add leading item support to the inline variant with appropriate styling
- Warn or throw if
hasLeadingItemis true whenvariant === 'inline'Additionally, the inline variant lacks visual feedback about active filter count (the drawer variant shows "Remove X filters" when filters are active). Consider adding a similar indicator for better UX consistency.
packages/ui/src/components/Filters/Filters.stories.tsx (1)
283-329: LGTM! Clean FiltersSection inline variant story.The story correctly demonstrates using the inline variant through
FiltersSectionwith proper state management and context.Minor note: This story displays current filters after the FiltersSection (lines 322-325), while
InlineVariantdisplays them before (lines 263-266). While this doesn't affect functionality, consider standardizing the layout for consistency across stories.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/ui/src/components/Filters/Filters.stories.tsx(1 hunks)packages/ui/src/components/Filters/Filters.tsx(2 hunks)packages/ui/src/components/Filters/Filters.types.ts(1 hunks)packages/ui/src/components/Filters/FiltersSection.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Filters/Filters.stories.tsx (3)
packages/ui/src/components/Filters/FiltersContext.tsx (1)
FiltersContextProvider(15-49)packages/ui/src/components/Filters/Filters.tsx (1)
Filters(32-194)packages/ui/src/components/Filters/FiltersSection.tsx (1)
FiltersSection(14-52)
🔇 Additional comments (4)
packages/ui/src/components/Filters/Filters.types.ts (1)
10-10: LGTM! Clean type addition.The optional
variantprop with a union type'drawer' | 'inline'is well-defined and maintains backward compatibility.packages/ui/src/components/Filters/FiltersSection.tsx (1)
21-21: LGTM! Proper prop forwarding.The
variantprop is correctly destructured and forwarded to theFilterscomponent, enabling the inline variant to work throughFiltersSection.Also applies to: 46-46
packages/ui/src/components/Filters/Filters.tsx (1)
38-38: LGTM! Correct default value.Setting
variant = 'drawer'as the default ensures backward compatibility with existing implementations.packages/ui/src/components/Filters/Filters.stories.tsx (1)
232-281: LGTM! Well-structured inline variant story.The story properly demonstrates the inline variant with:
- Appropriate state management using
useState- Context provider wrapping
- Current filters display for testing
- Correct variant prop usage
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@iAn-P1nt0 I saw you marked some conversations as resolved, but I don't see any changes in the code - did you perhaps forgot to push the changes? Changes from the comments still need to be applied - are you still open to working on this task? |
@marcinkrasowski I am attempting to resolve issues in this format for first time. Will try to resolve and notify on this thread. Thanks for your understanding. |
- Fix missing leadingItem: use items.map instead of filteredItems.map - Update container to responsive grid layout (1/2/3/4 columns) - Fix mobile layout: full-width filter rows with w-full class - Update action buttons: right-aligned on desktop, 50% max-width on mobile Resolves review comments from marcinkrasowski on PR o2sdev#365
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/components/Filters/Filters.tsx (1)
60-107: Inline variant wiring and layout look solid; consider a tiny refactor
- Formik setup,
countActiveFilters(values), andonSubmit(values)mirror the drawer behavior correctly, just without the sheet toggling.- Using
items.mapensures leading items are also rendered, which addresses the earlier issue where they were dropped in inline mode.- Layout matches prior feedback: single-column full-width rows on mobile (
grid-cols-1+w-full), multi-column grid on larger breakpoints, and actions right-aligned on desktop with buttons stretching to max 50% width on mobile (sm:justify-end,sm:max-w-[50%]).Minor optional cleanup:
separateLeadingItemis still computed above but unused in the inline path. You could short‑circuit earlier forvariant === 'inline'(or move theseparateLeadingItemcall into the drawer‑only branch) to avoid the extra work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/components/Filters/Filters.tsx(2 hunks)
🔇 Additional comments (2)
packages/ui/src/components/Filters/Filters.tsx (2)
32-40: Defaultingvariantto'drawer'preserves backward compatibilityAdding
variant = 'drawer'while keeping the rest of the signature intact is a clean way to introduce the inline mode without impacting existing callers.
110-199: Drawer variant behavior remains consistent with previous implementationThe drawer path retains its existing behavior: handling
leadingItemviaScrollContainer, usingactiveFilters+removeFilterscopy for the clear button, and wiring reset/submit through Formik withsetFiltersOpen(false)on submit. No regressions are apparent, and the new inline variant is cleanly gated by thevariantcheck above.
* Initial plan * feat: add inline variant to Filters component Authored-by: iAn-P1nt0 <[email protected]> ---------
- Fix missing leadingItem: use items.map instead of filteredItems.map - Update container to responsive grid layout (1/2/3/4 columns) - Fix mobile layout: full-width filter rows with w-full class - Update action buttons: right-aligned on desktop, 50% max-width on mobile Resolves review comments from marcinkrasowski on PR o2sdev#365
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
@iAn-P1nt0 looks good, will merge it once the reward and tip are issued, hopefully in the next couple of days, thanks for contributing :)
But as a final requirement before the reward, please leave feedback in a comment as it was mentioned in the original issue:
As an additional requirement, please provide a short feedback on your experiences with working with this framework - how easy or difficult it was to get started (including starting the project, getting around the monorepo or reading our docs) and to make the required changes.
Developer Experience Feedback on Open Self Service FrameworkThanks @marcinkrasowski for the opportunity to contribute! Here's my detailed feedback on working with the Open Self Service framework: ✅ Getting Started & Project SetupExperience: Smooth and straightforward The setup experience was excellent. The Positives:
✅ Monorepo Navigation & ArchitectureExperience: Very well organized Understanding the monorepo structure was surprisingly easy, thanks to excellent documentation on the project structure page. Strengths:
Minor observation:
✅ Documentation QualityExperience: Comprehensive and helpful The documentation is genuinely one of the framework's strongest points. It covers the critical paths well. What worked great:
Helpful details:
✅ Making Required ChangesExperience: Minimal friction, excellent code patterns Implementing the inline variant for the Filters component was straightforward. What made it smooth:
Specific workflow that helped:
Code quality observations:
🎯 What Contributed to Success
💡 Suggestions for Framework Enhancement
Final ThoughtsThe Open Self Service framework is well-engineered, thoughtfully documented, and developer-friendly. The monorepo structure is logical, the component patterns are consistent, and the development workflow with Storybook is highly productive. Contributing was genuinely enjoyable—the framework doesn't get in your way while maintaining strong architectural principles. Rating: 9/10 for developer experience ⭐ The framework strikes an excellent balance between being opinionated (good) and restrictive (bad), and the documentation quality is genuinely impressive for an open-source project. Thanks again for building such a solid framework and making the contribution process smooth! |
|
@iAn-P1nt0 thanks for that, and as the last task, would you mind resolving merge conflicts? |
|
@iAn-P1nt0 and reward and tip were just issued :) |
Resolved merge conflicts: - Filters.tsx: Kept PR changes for inline variant feature, fixed TypeScript error with 'isLeading' property check - FiltersSection.tsx: Kept PR changes for variant prop passing, fixed TypeScript error with 'isLeading' property check - package-lock.json: Accepted main's newer dependency versions and regenerated with npm install Technical changes: - Added 'in' operator checks for 'isLeading' property to handle FilterViewModeToggle type correctly - Updated dependencies to latest versions from main including: - @nestjs/cli: 11.0.10 → 11.0.14 - glob: 11.0.3 → 13.0.0 - typescript: 5.8.3 → 5.9.3 - webpack: 5.100.2 → 5.103.0 - @typescript-eslint packages: 8.46.4 → 8.48.1 - Storybook: 9.1.16 → 10.1.4 All builds passing successfully ✓
There was a problem hiding this comment.
Pull request overview
This PR adds an inline variant to the Filters component as an alternative to the existing drawer layout. However, there are several critical bugs and breaking changes that need to be addressed before merging.
Key Changes:
- Added
variant?: 'drawer' | 'inline'prop to FiltersProps with 'drawer' as default - Implemented inline rendering mode that displays filters in a grid layout with action buttons below
- Propagated variant prop through FiltersSection to Filters component
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Filters.types.ts | Added variant prop type definition to FiltersProps interface |
| FiltersSection.tsx | Propagates variant and hasLeadingItem props to Filters component |
| Filters.tsx | Implements inline variant with conditional rendering; removed FilterViewModeToggle handling; modified drawer variant button layout |
| Filters.stories.tsx | Added InlineVariant and InlineVariantWithSection story examples |
Critical Issues Found:
- The inline variant incorrectly uses
itemsinstead offilteredItems, causing leading items to be rendered twice - The inline variant completely ignores leading items, breaking expected behavior
- Removal of
FilterViewModeTogglespecial handling is a breaking change affecting multiple blocks across the codebase - Button order change in drawer variant may unexpectedly affect existing UX
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="flex flex-col gap-4"> | ||
| {/* Filters container - grid layout for desktop, full-width rows for mobile */} | ||
| <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 xl:grid-cols-4 gap-4"> | ||
| {items.map((item) => ( |
There was a problem hiding this comment.
The inline variant is using items directly instead of filteredItems. This means:
- When
hasLeadingItemis true, the leading item will be rendered twice (once in the inline grid and potentially elsewhere) - The separation logic in
separateLeadingItemis being bypassed
The drawer variant correctly uses filteredItems (line 170) to exclude the leading item from the drawer content. The inline variant should use filteredItems here to maintain consistent behavior.
| {items.map((item) => ( | |
| {filteredItems.map((item) => ( |
@marcinkrasowski merge conflicts resolved in the PR. Kindly let me know of any voluntary minor support required or paid task opportunities as when you plan them. |
2 participants
Initial plan
feat: add inline variant to Filters component
Authored-by: iAn-P1nt0 [email protected]
What does this PR do?
I have successfully implemented the inline variant for the Filters component as requested in issue #354. Here's what was accomplished:
Related Ticket(s)
feat: add inline variant to Filters component (#1) #365
Key Changes
✅ Implementation Complete
Changes Made:
variant?: 'drawer' | 'inline'toFiltersProps(defaults to'drawer')variantprop through toFilterscomponentdrawer: Existing behavior (button → sheet with filters)inline: Renders filters directly in flex container with wrapping, followed by action buttonsInlineVariantandInlineVariantWithSectionstoriesUsage
Screenshots
Inline variant
Inline variant with section
The implementation includes:
All filters displayed inline with proper wrapping
"Reset" and "Apply Filters" buttons at the bottom
Clean integration with FiltersSection for titles
Full backward compatibility with existing drawer variant
✅ All Acceptance Criteria Met
✅ Filters component accepts variant prop with values 'drawer' (default) or 'inline'
✅ When variant="inline", filters are rendered directly without a drawer
✅ Bottom row with "Apply" and "Reset" buttons is displayed
✅ FiltersSection correctly propagates the variant prop
✅ All existing functionality works correctly in both variants
✅ New variant is documented in Storybook with multiple examples
✅ Backward compatibility maintained (existing code works unchanged)
🎯 Minimal Changes Approach
Only 3 files were modified:
Filters.types.ts: Added variant prop definition
FiltersSection.tsx: Propagated variant prop (1 line added)
Filters.tsx: Added inline variant rendering logic (45 lines)
Filters.stories.tsx: Added 2 new story examples
How to test
Tested successfully in Storybook:
Verified drawer variant still works (backward compatibility)
Verified inline variant renders correctly
Confirmed filters wrap properly
Verified "Apply Filters" and "Reset" buttons work in both variants
The implementation is ready for review and merge!
Media (Loom or gif)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.