Skip to content

Picker - ExpandableOverlay imperativeCloseExpandable #3536

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
Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Picker - ExpandableOverlay imperativeCloseExpandable #3536

wants to merge 10 commits into from
+8 −4

Conversation

@adids1221
Copy link
Contributor

@adids1221 adids1221 commented Feb 26, 2025 •
edited
Loading

Description

Picker when passing onDismiss from customPickerProps the onDismiss getting called twice.
Fix new imperativeCloseExpandable function to pass to Modal as onDismiss instead of going into closeExpandable from the Picker and from the Modal
Snippet: (This issue occurs both on Modal and Dialog)

 <Picker
        placeholder="Favorite Language"
        value={language}
        onChange={item => setLanguage(item)}
        items={options}
        customPickerProps={{
          modalProps: {
            onDismiss: () => console.log('dismissed')
          }
        }}
      />

Implemented a state machine pattern to track the component's dismiss state.
Also fixed TODO task in Picker test.

Changelog

ExpandableOverlay internal usage of Modal onDismiss fix.

Additional Info

MADS-4656

@adids1221 adids1221 requested a review from M-i-k-e-l February 26, 2025 15:20
M-i-k-e-l
M-i-k-e-l reviewed Feb 27, 2025
View reviewed changes
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.

This feels overly complicated.
Any chance this can be solved in a simpler way?

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Feb 27, 2025
@adids1221
Copy link
Contributor Author

@M-i-k-e-l do you remember where did we used the imperativeCloseExpandable ?
was it the toggleExpandable or the useImperativeHandle ?

@adids1221 adids1221 requested a review from M-i-k-e-l March 18, 2025 16:00
@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Mar 18, 2025
@M-i-k-e-l
Copy link
Collaborator

@M-i-k-e-l do you remember where did we used the imperativeCloseExpandable ? was it the toggleExpandable or the useImperativeHandle ?

@adids1221
The changes were in ExpandableOverlay.
But it means all usages of the component needs to be tested (including private).

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Mar 19, 2025
@adids1221
Copy link
Contributor Author

@M-i-k-e-l do you remember where did we used the imperativeCloseExpandable ? was it the toggleExpandable or the useImperativeHandle ?

@adids1221 The changes were in ExpandableOverlay. But it means all usages of the component needs to be tested (including private).

I meant where in ExpandableOverlay, I think we changed the onDismiss props of the Modal and Dialog.
checking it.

@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Mar 20, 2025
@M-i-k-e-l
Copy link
Collaborator

@adids1221
This still has the old attempt

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Mar 23, 2025
@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Mar 23, 2025
@adids1221 adids1221 changed the title implement dismiss state management for closeExpandable control Picker - ExpandableOverlay imperativeCloseExpandable Mar 24, 2025
@M-i-k-e-l
Copy link
Collaborator

Didn't we need to talk about this? Or debug together?

@M-i-k-e-l M-i-k-e-l removed their assignment Mar 25, 2025
@adids1221
Copy link
Contributor Author

@M-i-k-e-l we spoke about it, you passed the imperativeHandle to the toggleExpandable but it's still calling onDismiss twice.
I'll set us another meeting about that.

@M-i-k-e-l
Copy link
Collaborator

@adids1221 the solution we've talked about is working correctly on iOS but not on Android (no onDismiss there).

  1. Please rename the function.
  2. Did you test it with screenreader? Is the focusAccessibility working correctly?

@adids1221
Copy link
Contributor Author

@M-i-k-e-l
Iv'e checked it on my device with screenreader.
focusAccessibility should be part of both imperativeCloseExpandable and closeExpandable.
closeExpandable is getting called by Dialog and it's working as expected.
But when when using imperativeCloseExpandable from Dialog the test says the onDismiss isn't getting called.

Sumup:
Modal should use imperativeCloseExpandable, Dialog closeExpandable and both of them should have focusAccessibility.

Let's sync about it

@adids1221
Copy link
Contributor Author

@M-i-k-e-l iv'e checked accessibility both on IOS and Android, working fine for me.
I noticed something in IOS that the Modal.TopBar rendered even if it's empty (but only IOS I guess something on my device).

@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 May 8, 2025
@adids1221
Copy link
Contributor Author

Moved to draft.
TODO: check in all the components that are using ExpendableOverlay

@adids1221 adids1221 marked this pull request as draft May 19, 2025 08:28
@adids1221 adids1221 assigned adids1221 and unassigned M-i-k-e-l May 19, 2025
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 Awaiting requested review from M-i-k-e-l

At least 1 approving review is required to merge this pull request.

Assignees

@adids1221 adids1221

Labels

waiting-for-response

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

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