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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

adids1221
Copy link
Contributor

@adids1221 adids1221 commented Feb 26, 2025

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

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).

@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.

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

@adids1221
This still has the old attempt

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants