Skip to content

Infra/picker expandable overlay #1632

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Nov 14, 2021
Merged

Infra/picker expandable overlay #1632

merged 10 commits into from
Nov 14, 2021

Conversation

ethanshar
Copy link
Collaborator

@ethanshar ethanshar commented Oct 31, 2021 •
edited
Loading

Description

Please review but don't merge !!!

This refactor in Picker component purpose is to stop using old TextField expandable API (what controls the picker modal show/hide) and instead use our new ExpandableOverlay component that is dedicated solely for that

This PR has few goals

  • Gradually disconnect Picker from old TextField in preparation for using the new TextField
  • Using a better infrastructure for the expandable overlay that supports show either modal or dialog - which will help us in private Picker

Few notes

  • This PR is not perfect, I left some code in comment in purpose
  • I ran over all use cases in example screen and made sure they work and still supported
  • I add some future notes for things that we better deprecate but didn't really handle any of them in this PR
  • I removed a failing test that is not relevant anymore and TBH didn't test much to begin with

Changelog

Refactor Picker component internally using Incubator.ExpandableOverlay for handling the Picker modal

@ethanshar ethanshar requested a review from M-i-k-e-l October 31, 2021 15:13
M-i-k-e-l
M-i-k-e-l reviewed Nov 1, 2021
View reviewed changes
@ethanshar ethanshar requested a review from M-i-k-e-l November 3, 2021 06:24
Inbal-Tish
Inbal-Tish reviewed Nov 3, 2021
View reviewed changes
src/incubator/expandableOverlay/index.tsx Outdated
topBarProps,
renderCustomOverlay,
...others
} = props;
const [expandableVisible, setExpandableVisible] = useState(false);
const showExpandable = useCallback(() => setExpandableVisible(true), []);
const hideExpandable = useCallback(() => setExpandableVisible(false), []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to invoke the Modal/Dialog's 'onDismiss' in 'hideExpandable' so the user can pass a callback.
And maybe Modal's 'onShow' in 'showExpandable'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

M-i-k-e-l
M-i-k-e-l reviewed Nov 3, 2021
View reviewed changes
@ethanshar ethanshar added the Important for Next Release PR that must be included in the release version label Nov 14, 2021
@ethanshar ethanshar merged commit a488b24 into master Nov 14, 2021
@ethanshar ethanshar deleted the infra/Picker_ExpandableOverlay branch April 18, 2022 09:00
@snyk-bot snyk-bot mentioned this pull request Feb 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M-i-k-e-l M-i-k-e-l M-i-k-e-l approved these changes

@Inbal-Tish Inbal-Tish Awaiting requested review from Inbal-Tish

Assignees

@M-i-k-e-l M-i-k-e-l

Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethanshar @Inbal-Tish @M-i-k-e-l