feat: updated filters dropdown and created stories for it #3174
Conversation
devilkiller-ag
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
akshatnema
added
the
gsoc
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3174--asyncapi-website.netlify.app/ |
|
@devilkiller-ag Where are stories of dropdown? |
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
Added all the stories for filters dropdown. |
|
/update |
…ate-filters-dropdown
…ate-filters-dropdown
WalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (3)
components/tools/FiltersDropdown.stories.tsx (2)
1-2: Consider importing directly from '@storybook/react'Since the
useArgshook is also available from@storybook/react, double-check if you want to rely on@storybook/preview-apior the more standard@storybook/react. Consistency across Storybook imports may simplify maintenance.
29-51: Use consistent story naming conventionsCurrently, the primary story is defined as
Dropdown. For clarity and discoverability, consider naming itDefaultDropdownor something similarly descriptive. This ensures better readability in the Storybook UI alongside your specialized stories.- const Dropdown: Story = { + const DefaultDropdown: Story = {components/tools/FiltersDropdown.tsx (1)
Line range hint
29-47: Ensure event usage is necessary
handleClickOptionincludes aneventparameter. Currently, the function references onlycheckedOptionsandoption. If you don’t needeventfor specific logic such as stopping propagation or preventing default behavior, it could be removed to simplify the function signature.- const handleClickOption = (event: React.MouseEvent<HTMLDivElement, MouseEvent>, option: string) => { + const handleClickOption = (_: React.MouseEvent<HTMLDivElement, MouseEvent>, option: string) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/img/illustrations/icons/CheckedIcon.svgis excluded by!**/*.svgpublic/img/illustrations/icons/UncheckedIcon.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
components/tools/FiltersDropdown.stories.tsx(1 hunks)components/tools/FiltersDropdown.tsx(3 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR testing - if Node project
package.json
[error] Package lock file is out of sync. Missing dependencies in lock file: @storybook/[email protected] and @storybook/[email protected]
[warning] Package '@tisoap/[email protected]' has incompatible engine requirements. Required: node >= 16, npm ^8.0.0. Current: node v20.11.0, npm 10.2.4
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (5)
components/tools/FiltersDropdown.stories.tsx (2)
9-25: Good use ofargTypesfor controlling props in StorybookDefining
argTypesfordataList,checkedOptions, andsetCheckedOptionshelps maintain clarity around how each prop is expected to be controlled. This is a beneficial setup for customizing story interactions.
53-75: Check for potential overlap between Language and Technology listsWhile the
LanguageDropdownandTechnologyDropdownstories provide separate data sets, confirm whether there's any overlap or potential naming collisions (e.g., a technology that is also a recognized language) that might produce confusion when searching or filtering. If so, consider unique naming or additional context in the UI.components/tools/FiltersDropdown.tsx (2)
8-8: ExportingFiltersDropdownPropsfacilitates reusabilityExporting the interface is valuable, especially if you plan to reuse this dropdown across multiple stories or even other components. This is a positive change in making the codebase more modular.
46-61: Good approach for toggling checked stateUsing a single click event and toggling the presence of
optionincheckedOptionsis concise. Maintaining a consistent, minimal approach to toggling states helps keep the UI responsive and straightforward.package.json (1)
58-58: Confirm Storybook addons integrationAdding
"@storybook/addons": "^7.6.17"helps expand Storybook's capabilities. However, ensure it aligns with the other Storybook packages’ versions. Mismatched versions can cause unexpected behavior or build conflicts.🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] Package lock file is out of sync. Missing dependencies in lock file: @storybook/[email protected] and @storybook/[email protected]
[warning] Package '@tisoap/[email protected]' has incompatible engine requirements. Required: node >= 16, npm ^8.0.0. Current: node v20.11.0, npm 10.2.4
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3174 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 732 732
=========================================
Hits 732 732 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/tools/FiltersDropdown.tsx (3)
38-40: Handle empty data list gracefully.While the styling updates look good, consider short-circuiting or providing a placeholder message when
dataListis empty to improve user feedback.return ( dataList.length === 0 ? ( + <div>No filter options available</div> ) : ( <div className={twMerge( `max-w-lg flex flex-col max-h-[40vh] gap-1 overflow-y-auto p-2 px-0 duration-200 delay-150 bg-gray-200 ${className}` )} data-testid='FiltersDropdown-div' > ... </div> ) );
46-53: Improve accessibility by using a semantically interactive element.It’s recommended to use a
<button>or at least add arole="button"to the clickable<div>elements. This ensures proper focus order and keyboard interaction for users relying on assistive technologies.-<div +<button key={index} - className={twMerge( - ... - )} - onClick={(event) => handleClickOption(event, data.name)} + role="button" + type="button" + onClick={(event) => handleClickOption(event, data.name)} > ... -</div> +</button>
54-61: Consider text alternatives for icons.Using images for checked/unchecked states is fine, but ensure meaningful alt text (e.g., “Selected option” and “Unselected option”) to enhance screen-reader support for accessible user experiences.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
components/tools/FiltersDropdown.tsx(3 hunks)
🔇 Additional comments (2)
components/tools/FiltersDropdown.tsx (2)
8-8: Encapsulating props for broader usage.Exporting the
FiltersDropdownPropsinterface extends its utility across the codebase, promoting reusability and consistency.
29-29: Good practice: capturing mouse event details.Adding the
eventparameter allows more control over the user interaction, such as preventing default actions or stopping event propagation where necessary.
|
@akshatnema this PR is ready for review! |
|
@akshatnema I have made the following changes:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/tools/FiltersDropdown.tsx (1)
Line range hint
29-34: Consider removing unused event parameter.The
eventparameter is added to the function signature but not used within the function body. If it's not needed for future functionality, consider removing it to keep the code clean.- const handleClickOption = (event: React.MouseEvent<HTMLDivElement, MouseEvent>, option: string) => { + const handleClickOption = (option: string) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/tools/Filters.tsx(8 hunks)components/tools/FiltersDropdown.tsx(3 hunks)
🧰 Additional context used
🪛 ESLint
components/tools/Filters.tsx
[error] 27-27: Delete ;
(prettier/prettier)
[error] 40-40: Replace OpenedFiltersDropdownType.NONE with ⏎····OpenedFiltersDropdownType.NONE⏎··
(prettier/prettier)
[error] 190-190: Expected '===' and instead saw '=='.
(eqeqeq)
[error] 193-197: Unexpected negated condition.
(no-negated-condition)
[error] 196-196: Insert ;
(prettier/prettier)
[error] 196-197: Missing semicolon.
(semi)
[error] 208-208: Replace ·className={my-auto·${openedFiltersDropown·==·OpenedFiltersDropdownType.LANGUAGE·?·'rotate-180'·:·''}} with ⏎················className={my-auto·${openedFiltersDropown·==·OpenedFiltersDropdownType.LANGUAGE·?·'rotate-180'·:·''}}⏎·············
(prettier/prettier)
[error] 208-208: Expected '===' and instead saw '=='.
(eqeqeq)
[error] 210-210: Expected '===' and instead saw '=='.
(eqeqeq)
[error] 239-243: Unexpected negated condition.
(no-negated-condition)
[error] 242-242: Insert ;
(prettier/prettier)
[error] 242-243: Missing semicolon.
(semi)
[error] 254-254: Replace ·className={my-auto·${openedFiltersDropown·===·OpenedFiltersDropdownType.TECHNOLOGY·?·'rotate-180'·:·''}} with ⏎················className={my-auto·${openedFiltersDropown·===·OpenedFiltersDropdownType.TECHNOLOGY·?·'rotate-180'·:·''}}⏎·············
(prettier/prettier)
[error] 285-289: Unexpected negated condition.
(no-negated-condition)
[error] 288-288: Insert ;
(prettier/prettier)
[error] 288-289: Missing semicolon.
(semi)
[error] 300-300: Replace ·className={my-auto·${openedFiltersDropown·===·OpenedFiltersDropdownType.CATEGORY·?·'rotate-180'·:·''}} with ⏎················className={my-auto·${openedFiltersDropown·===·OpenedFiltersDropdownType.CATEGORY·?·'rotate-180'·:·''}}⏎·············
(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
components/tools/Filters.tsx
[error] 27-27: Delete semicolon
[error] 40-40: Incorrect formatting of OpenedFiltersDropdownType.NONE
[error] 190-190: Expected '===' and instead saw '=='
[error] 193-193: Unexpected negated condition
[error] 196-196: Missing semicolon
[error] 208-208: Incorrect formatting of className and expected '===' instead of '=='
[error] 210-210: Expected '===' and instead saw '=='
[error] 239-239: Unexpected negated condition
[error] 242-242: Missing semicolon
[error] 254-254: Incorrect formatting of className
[error] 285-285: Unexpected negated condition
[error] 288-288: Missing semicolon
[error] 300-300: Incorrect formatting of className
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (3)
components/tools/Filters.tsx (1)
22-27: Great improvement in state management!The introduction of
OpenedFiltersDropdownTypeenum to manage dropdown states is a clean and maintainable solution, replacing multiple boolean states with a single state variable.Also applies to: 40-40
🧰 Tools
🪛 ESLint
[error] 27-27: Delete
;(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 27-27: Delete semicolon
components/tools/FiltersDropdown.tsx (2)
Line range hint
8-13: LGTM! Well-structured props interface.The exported interface is well-typed and properly documented, making it reusable for stories and tests.
38-40: Consider accessibility implications of max-height constraint.The max height constraint of
20vhwith overflow scrolling might impact accessibility:
- Users with motor impairments might find it difficult to scroll through a small container.
- The content might be cut off on smaller screens.
Consider:
- Using a larger max height on mobile devices
- Adding keyboard navigation support
- Ensuring sufficient contrast for the scrollbar
components/tools/Filters.tsx
Outdated
| onClick={() => { | ||
| if (openedFiltersDropown !== OpenedFiltersDropdownType.LANGUAGE) { | ||
| setOpenedFiltersDropown(OpenedFiltersDropdownType.LANGUAGE); | ||
| } else { | ||
| setOpenedFiltersDropown(OpenedFiltersDropdownType.NONE) | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated dropdown toggle logic.
The dropdown toggle logic is duplicated across three locations. Consider extracting this into a reusable function to improve maintainability and reduce code duplication.
Here's a suggested implementation:
+ const toggleDropdown = (dropdownType: OpenedFiltersDropdownType) => {
+ setOpenedFiltersDropown(
+ openedFiltersDropown === dropdownType
+ ? OpenedFiltersDropdownType.NONE
+ : dropdownType
+ );
+ };
- onClick={() => {
- if (openedFiltersDropown !== OpenedFiltersDropdownType.LANGUAGE) {
- setOpenedFiltersDropown(OpenedFiltersDropdownType.LANGUAGE);
- } else {
- setOpenedFiltersDropown(OpenedFiltersDropdownType.NONE)
- }
- }}
+ onClick={() => toggleDropdown(OpenedFiltersDropdownType.LANGUAGE)}Apply similar changes for Technology and Category dropdowns.
Also applies to: 238-244, 284-290
🧰 Tools
🪛 ESLint
[error] 193-197: Unexpected negated condition.
(no-negated-condition)
[error] 196-196: Insert ;
(prettier/prettier)
[error] 196-197: Missing semicolon.
(semi)
🪛 GitHub Actions: PR testing - if Node project
[error] 193-193: Unexpected negated condition
[error] 196-196: Missing semicolon
components/tools/Filters.tsx
Outdated
| @@ -182,9 +187,15 @@ | |||
| <div className='w-full'> | |||
| <div | |||
| className={twMerge( | |||
| `px-4 py-2 flex justify-between rounded-lg border border-gray-400 w-full bg-gray-200 text-gray-700 shadow text-sm cursor-pointer ${openLanguage ? 'rounded-b-none' : ''}` | |||
| `px-4 py-2 flex justify-between rounded-lg border border-gray-400 w-full bg-gray-200 text-gray-700 shadow text-sm cursor-pointer ${openedFiltersDropown == OpenedFiltersDropdownType.LANGUAGE ? 'rounded-b-none' : ''}` | |||
There was a problem hiding this comment.
Fix code style issues to pass CI checks.
Several code style issues need to be addressed:
- Replace loose equality operators (
==) with strict equality (===) - Add missing semicolons
Apply these fixes:
- `${openedFiltersDropown == OpenedFiltersDropdownType.LANGUAGE`
+ `${openedFiltersDropown === OpenedFiltersDropdownType.LANGUAGE`
- setOpenedFiltersDropown(OpenedFiltersDropdownType.NONE)
+ setOpenedFiltersDropown(OpenedFiltersDropdownType.NONE);Also applies to: 208-208, 210-210, 196-196, 242-242, 288-288
🧰 Tools
🪛 ESLint
[error] 190-190: Expected '===' and instead saw '=='.
(eqeqeq)
🪛 GitHub Actions: PR testing - if Node project
[error] 190-190: Expected '===' and instead saw '=='
| return ( | ||
| <div | ||
| key={index} | ||
| className={twMerge( | ||
| `hover:bg-gray-300 text-black p-1 py-2 gap-1 flex cursor-pointer items-start ${checked ? 'bg-gray-400' : ''}` | ||
| )} | ||
| onClick={(event) => handleClickOption(event, data.name)} | ||
| > | ||
| {checked ? ( | ||
| <img src='/img/illustrations/icons/CheckedIcon.svg' alt='checked' /> | ||
| ) : ( | ||
| <img src='/img/illustrations/icons/UncheckedIcon.svg' alt='unchecked' /> | ||
| )} | ||
| <div className='-mt-px mb-px text-xs'>{data.name}</div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and maintainability.
Several improvements could be made to the rendering logic:
- Use semantic HTML:
- <div
+ <button
key={index}
className={twMerge(
- `hover:bg-gray-300 text-black p-1 py-2 gap-1 flex cursor-pointer items-start ${checked ? 'bg-gray-400' : ''}`
+ `hover:bg-gray-300 text-black p-1 py-2 gap-1 flex items-start ${checked ? 'bg-gray-400' : ''}`
)}
- onClick={(event) => handleClickOption(event, data.name)}
+ onClick={() => handleClickOption(data.name)}
- >
+ type="button"
+ aria-pressed={checked}
+ >- Extract icon paths as constants:
const ICON_PATHS = {
checked: '/img/illustrations/icons/CheckedIcon.svg',
unchecked: '/img/illustrations/icons/UncheckedIcon.svg'
} as const;- Consider increasing text size for better readability:
- <div className='-mt-px mb-px text-xs'>{data.name}</div>
+ <div className='-mt-px mb-px text-sm'>{data.name}</div>
Description
This PR updates the filters dropdown according to the design proposed in issue #1318 and creates stories for it.
Related issue(s)
Resolves #1318
Summary by CodeRabbit
New Features
Improvements
Chores
Dependency Updates