Skip to content

Feat/picker use dialog 2 #2804

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

Merged
merged 20 commits into from
Nov 21, 2023
Merged

Feat/picker use dialog 2 #2804

merged 20 commits into from
Nov 21, 2023

Conversation

ethanshar
Copy link
Collaborator

@ethanshar ethanshar commented Nov 13, 2023

Description

This PR based on PR Adi started working for a fix for Web.
We're adding some support for features that we later use in private's Picker.
useDialog - ability to show Picker as dialog
renderCustomDialogHeader - support rendering a custom dialog header by passing a callback. We pass to the callback to onDone, onCancel of the Picker so the user (us in this case) can do what they wish

Changelog

Support rendering Picker as dialog and also render custom dialog header

Additional info

@ethanshar ethanshar requested a review from M-i-k-e-l November 16, 2023 14:33
@ethanshar ethanshar added the Important for Next Release PR that must be included in the release version label Nov 19, 2023
enableModalBlur={false}
onChange={item => this.setState({option: item})}
topBarProps={{title: 'Languages'}}
useDialog
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it's related to the PR, but passing useSafeArea does not work together with useDialog

  • It can be passed via customPickerProps.dialogProps

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the flexness issue of the FlatList inside the dialog

searchPlaceholder={'Search a language'}
>
{_.map(dialogOptions, option => (
<Picker.Item key={option.value} value={option.value} label={option.label} disabled={option.disabled}/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last items are not properly scrollable \ visible

testID
} = props;
const context = useContext(PickerContext);
const [wheelPickerValue, setWheelPickerValue] = useState<PickerSingleValue>(context.value ?? items?.[0].value);
const wrapperContainerStyle = useMemo(() => {
const shouldFlex = Constants.isWeb ? 1 : useDialog ? 0 : 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it should be only flex

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didnt change the name, but changed the logic

)}
</ExpandableOverlay>
{
// @ts-expect-error
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not {/* @ts-expect-error */}? (I assume the whole change here is due to alignment)

@ethanshar ethanshar requested a review from M-i-k-e-l November 19, 2023 09:51
@ethanshar
Copy link
Collaborator Author

@M-i-k-e-l ping on this one

Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a small suggetsion

Comment on lines +42 to +43
const shouldFlex = true;
const style = {flex: shouldFlex ? 1 : 0, maxHeight: Constants.isWeb ? Constants.windowHeight * 0.75 : undefined};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const shouldFlex = true;
const style = {flex: shouldFlex ? 1 : 0, maxHeight: Constants.isWeb ? Constants.windowHeight * 0.75 : undefined};
const style = {flex: 1, maxHeight: Constants.isWeb ? Constants.windowHeight * 0.75 : undefined};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants