-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DS-497] - Create dropdown input component 🚀 #586
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is used only countries ? For me OK! Just fix the spacing and sizes with a Yoga Theming spacing e all done! Congrats for that contrib.
const renderListCountries = () => { | ||
return countries.map(country => { | ||
const isCountrySelected = selectedCountry.id === country.id; | ||
|
||
return ( | ||
<ItemDropDown | ||
d="flex" | ||
alignItems="center" | ||
justifyContent="flex-start" | ||
alignContent="center" | ||
key={country.id} | ||
onClick={changeSelectedCountry(country)} | ||
> | ||
<ContainerName> | ||
<Box> | ||
<Icon | ||
as={country.icon} | ||
width="medium" | ||
height="medium" | ||
marginRight={4} | ||
/> | ||
</Box> | ||
<Text.Small | ||
fw={isCountrySelected ? 'medium' : 'regular'} | ||
color={isCountrySelected ? 'primary' : 'secondary'} | ||
> | ||
{country.name} | ||
</Text.Small> | ||
</ContainerName> | ||
{isCountrySelected && ( | ||
<Icon as={Check} width="medium" height="medium" fill="vibin" /> | ||
)} | ||
</ItemDropDown> | ||
); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating this renderListCountries
function is an anti-pattern.
It's better to do the .map
directly and (optionally) extract <IterDropDown...
to a new component outside render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont' forget the solution we've discussed offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
|
||
const ContainerName = styled.div``; | ||
|
||
const DropdownInput = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this component is too the generic, considering we have this one:
https://gympass.github.io/yoga/components/dropdown
So, WDYT of?
const DropdownInput = ({ | |
const CountryDropdownInput = ({ |
value, | ||
selectedCountry, | ||
onChangeSelectedCountry, | ||
countries, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you choose the path of specifying this components in only to work with countries
this list can be static instead of a prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, whoever uses the component will not be able to specify the countries that should be displayed.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently gympass only supports the countries we have flags for. If you want to keep this property, then you should make it optional and have a default list...
const renderListCountries = () => { | ||
return countries.map(country => { | ||
const isCountrySelected = selectedCountry.id === country.id; | ||
|
||
return ( | ||
<ItemDropDown | ||
d="flex" | ||
alignItems="center" | ||
justifyContent="flex-start" | ||
alignContent="center" | ||
key={country.id} | ||
onClick={changeSelectedCountry(country)} | ||
> | ||
<ContainerName> | ||
<Box> | ||
<Icon | ||
as={country.icon} | ||
width="medium" | ||
height="medium" | ||
marginRight={4} | ||
/> | ||
</Box> | ||
<Text.Small | ||
fw={isCountrySelected ? 'medium' : 'regular'} | ||
color={isCountrySelected ? 'primary' : 'secondary'} | ||
> | ||
{country.name} | ||
</Text.Small> | ||
</ContainerName> | ||
{isCountrySelected && ( | ||
<Icon as={Check} width="medium" height="medium" fill="vibin" /> | ||
)} | ||
</ItemDropDown> | ||
); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont' forget the solution we've discussed offline
e51856f
to
2d62285
Compare
Description 📄
Context:
(WHAT) What does this component do?
(WHY) That's been required by some teams.
Platforms 📲
Type of change 🔍
How Has This Been Tested? 🧪
Checklist: 🔍
Screenshots 📸
dropdowninput.mov