[CSL-3157] Sort Component #62
Conversation
CSL-3157 [MVP] Create a Sorting Component
A Component that renders out Sort functionalities OOTB Definition of done:
Additional Notes: |
esezen
left a comment
esezen
left a comment
There was a problem hiding this comment.
The actual implementation looks great. I left some comments around CSS
src/components/Sort/sort.css
Outdated
| @@ -0,0 +1,84 @@ | |||
| .plp-sort { | |||
| background-color: #FFFFFF; | |||
| font-family: 'Nunito Sans', 'Helvetica Neue', Helvetica, Arial, sans-serif; | |||
There was a problem hiding this comment.
Is this required? Can we stick to the default font?
There was a problem hiding this comment.
I was matching Figma designs and our Button component already uses this font.
Should we update the default font to be
font-family: 'Nunito Sans', 'Helvetica Neue', Helvetica, Arial, sans-serif;
wdyt?
There was a problem hiding this comment.
I think we can default to 'Inter', sans-serif like how we do for the other ui-libraries. I believe I've merged this into main already, so I think we can remove this.
src/components/Sort/sort.css
Outdated
| transition: 0.25s ease; | ||
| } | ||
|
|
||
| .plp-sort label span:before { |
There was a problem hiding this comment.
Do you mind adding some comments here to explain what's happening here?
src/components/Sort/Sort.tsx
Outdated
| changeSelectedSort, | ||
| }) | ||
| ) : ( | ||
| <div className='plp-sort'> |
There was a problem hiding this comment.
Can we prefix all classes with cio-?
src/components/Sort/Sort.tsx
Outdated
| <div className='plp-sort'> | ||
| <button type='button' className='collapsible' onClick={toggleCollapsible}> | ||
| Sort | ||
| <i className={`arrow ${isOpen ? 'up' : 'down'}`} /> |
There was a problem hiding this comment.
What do you think about repeating the arrrow part here to make sure we don't treat these as 2 separate classes? So instead of arrow up it would be arrow-up
| width: 20px; | ||
| height: 20px; | ||
| border-radius: 50%; | ||
| transition: 0.25s ease; |
There was a problem hiding this comment.
The animation on these are so cool 🚀
Mudaafi
left a comment
Mudaafi
left a comment
There was a problem hiding this comment.
Love the implementation, had a couple of notes but great work so far
src/utils/transformers.ts
Outdated
| return transformedItem; | ||
| } | ||
|
|
||
| export function transformSortOptions(options?: Partial<SortOption>[]): PlpSortOption[] { |
There was a problem hiding this comment.
I see you've renamed this by removing the response but wdyt of transformResponseSortOptions?
src/utils/transformers.ts
Outdated
| facets: response.facets, | ||
| groups: response.groups, | ||
| sortOptions: response.sort_options, | ||
| sortOptions: transformSortOptions(response.sort_options), |
There was a problem hiding this comment.
Loved that you transformed it here!
| import { Meta, Markdown, ArgTypes } from '@storybook/blocks'; | ||
| import * as SortStories from './Sort.stories'; | ||
|
|
||
| <Meta of={SortStories} /> |
There was a problem hiding this comment.
I like the simplicity of this but I feel like our current props table is a little non-descriptive. Could we add descriptions to the props and what they're used for? i.e. isOpen is the default state and searchAndBrowseResponse is mainly for SSR?
|
LGTM! Thanks Hossam, I'll merge it |
Introducing a Sort Component that takes in a search or a browse response and render sort options dynamically:
[GIF]