Feat: Added Picker component #204
Conversation
|
Deploy preview for kiwicom-universal-components ready! Built with commit 4c3650d https://deploy-preview-204--kiwicom-universal-components.netlify.com |
|
This is in the storybook preview. There might be a small issue. Could you look at it @JoseDuSV ?
|
JosefDuda
force-pushed
the
feat/picker
branch
from
daa059b
to
2b34ba4
Compare
Thanks, it's fixed now. |
|
@JoseDuSV There is a conflict in Also, I am wondering about using the native browser UI for the web version. Would it be hard to do something like on the kiwi.com page? |
JosefDuda
force-pushed
the
feat/picker
branch
from
2b34ba4
to
33319ef
Compare
|
@RobinCsl Conflict solved. And you are right, there is definitely room for improvement in web version. For now there is native select used in passenger form section even on kiwi (only button style is slightly different).
But we definitely don't need to strictly follow it and we can make the select items style nicer and closer to class selector on homepage.
Can we just merge this PR to make component available and I will create extra issue for web version improvements. Thanks |
JosefDuda
force-pushed
the
feat/picker
branch
from
33319ef
to
46375c4
Compare
src/Picker/Picker.ios.js
Outdated
| render() { | ||
| const { open, selectedValue, pickerValue } = this.state; | ||
| const { optionsData } = this.props; | ||
| const selectedOption = optionsData.find( |
There was a problem hiding this comment.
This logic, and the corresponding component is duplicated between ios and android. Could you extract it?
There was a problem hiding this comment.
Whole duplicate native picker implementation extracted to separate component and duplicate function added to PickerHelpers.
src/Picker/Picker.ios.js
Outdated
| > | ||
| {pickerOptions} | ||
| </RNPicker> | ||
| <Button onPress={this.handleOkPress} type="secondary" label="OK" /> |
There was a problem hiding this comment.
I am not sure about this label. I guess OK is pretty international. But we really need to be wary of this. What could we do about it?
There was a problem hiding this comment.
Label extracted to props as in other cases.
|
|
||
| import type { Props } from './PickerTypes'; | ||
|
|
||
| const DEFAULT_OPTION = 'default-empty-option'; |
There was a problem hiding this comment.
Could you explain what this achieves?
There was a problem hiding this comment.
Sure. Short description added to const. Hope it will be clearer now.
JosefDuda
changed the title
|
Also, this is not strictly related to this PR, but we will need to find a way to properly pass prop types to the components in MDX files as Docz does not parse them if they are outside. |
JosefDuda
force-pushed
the
feat/picker
branch
2 times, most recently
from
f631736
to
2142dae
Compare
Summary: Picker implementation for each platform. Will be used for example as nationality selector in passenger section of booking screen.
JosefDuda
force-pushed
the
feat/picker
branch
from
2142dae
to
4c3650d
Compare
JosefDuda
changed the title
|
iOS version now uses modal to show options. So the implementation is more flexible than in first iteration.
|
| expect(getByText(testData[1].label)).toBeDefined(); | ||
| }); | ||
|
|
||
| it('renders', () => { |
There was a problem hiding this comment.
Sometimes I wonder of the usefulness and value of these shallow snapshots. Maybe the previous test is enough to assert that your component at least rendered the right label.
Summary: Picker implementation for each platform. Will be used for example as nationality selector in passenger section of booking screen.
Related to:
kiwicom/margarita#73
kiwicom/margarita#121
Desktop:
iOS:
Android:
