feat: implement phone input component #1533
Conversation
…to implement-phone-input-component
…to implement-phone-input-component
…to implement-phone-input-component
Features
Bug Fixes
Contributors |
yvmunayev
changed the title
src/components/PhoneInput/helpers/__test__/findCountryByIsoCode.spec.js
Outdated
Show resolved
Hide resolved
|
|
||
| const StyledIndicator = styled.span` | ||
| z-index: 2; | ||
| margin-top: -4px; |
There was a problem hiding this comment.
Negative margins is bad practice, also top and bottom margin has no effect on inline elements
|
@yvmunayev There is a conflict with the |
|
When pass an empty array to the countries prop it gets broken, when fix it also add a test for this situation |
| import { useSimulatedLoading } from './hooks'; | ||
|
|
||
| const CountriesList = memo(props => { | ||
| const { countries, country, itemsRef, handleCountryChange, handleActiveChange } = props; |
There was a problem hiding this comment.
normally handle is used to define the function that handles the callback, instead this should be called onCountryChange and onActiveChange, I think it is better simply call them onChange and onHover
| onClick={() => handleCountryChange(value)} | ||
| onMouseEnter={() => handleActiveChange(index)} | ||
| role="option" | ||
| aria-selected={false} |
There was a problem hiding this comment.
aria-selected will be always false?
| if (isFilteredCountry) { | ||
| filteredCountries.unshift(country); | ||
| } | ||
| return filteredCountries; |
There was a problem hiding this comment.
take look here: src/components/InternalDropdown/helpers/searchFilter.js
this is a filter function that we use in other projects, maybe we can do similar and simplify this
| itemsRef, | ||
| handleCountryChange, | ||
| setFocusIndex, | ||
| ); |
There was a problem hiding this comment.
for now leave this, but we should avoid pass to much arguments to a function, instead, when they are more than two we pass an object (only one argument with named parameters)
…to implement-phone-input-component
| @@ -1,6 +1,6 @@ | |||
| import styled from 'styled-components'; | |||
| import attachThemeAttrs from '../../../../styles/helpers/attachThemeAttrs'; | |||
| import MenuArrowButton from '../../../Picklist/menuArrowButton'; | |||
| import CheckmarkIcon from '../../../Option/checkmark'; | |||
There was a problem hiding this comment.
I think we should not get this icon from other component folders since changes to Options could break PhoneInput, in this case it is better to repeat the icon to be inside PhoneInput folder, we are working in a solution in order to not repeat icons any more
fix: #1467
Changes proposed in this PR:
feat: implement PhoneInput component
fix: add useCallback to useOutsideClick
I have followed (at least) the PR section of the contributing guide.
@nexxtway/react-rainbow