Skip to content
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

Scrollable modals #205

Merged
merged 8 commits into from
Sep 14, 2023
Merged

Conversation

nop33
Copy link
Member

@nop33 nop33 commented Sep 13, 2023

  • Every ModalContent is now an instance of ScrollView (from gesture handler)
  • A new component called ModalFlatListContent can be used instead of ModalContent for rendering modals that include a FlatList (also from gesture handler)
  • Unless the maximisedContent or the customMinHeight prop is set, the BottomModal will take the height of its contents.

This doesn't work well for minimized modals. Needs investigation. It does!

@nop33 nop33 requested a review from mvaivre September 13, 2023 16:16
@nop33 nop33 changed the base branch from UI-refinements-ilias to UI-refinements September 14, 2023 07:59
padding-top: 10px;
padding-bottom: 10px;
`
const getDefaultProps = ({ verticalGap, contentContainerStyle }: ModalContentProps) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the contentContainerStyle properties isn't used in this file (only verticalGap? is passed to getDefaultProps`). You wanted to keep it for potential future use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! 😅

@mvaivre
Copy link
Member

mvaivre commented Sep 14, 2023

A couple of things I'm noticing:

  • I still don't have any closing animation when selecting an address or a contact in the Destination screen (not a huge deal). After closing, I don't see any opening animation either.
  • I notice a jump down when swiping the Transaction modal up to expand. As if it first quickly wants to minimise before expanding. When swiping down to minimise, it's the contrary. It seems to first expand then minimize.
  • The transaction modal lacks the vertical gap (margin below title)
  • There's a right margin next to the address selection modal opened from the AddressesScreen.

Other than this it works well. One last suggestion: we could maybe stop using maximisedContent for the currency and network settings in the Settings modal. WDYT?

@nop33
Copy link
Member Author

nop33 commented Sep 14, 2023

I still don't have any closing animation when selecting an address or a contact in the Destination screen (not a huge deal). After closing, I don't see any opening animation either.

Fixed ✅

The transaction modal lacks the vertical gap (margin below title)

Fixed ✅

There's a right margin next to the address selection modal opened from the AddressesScreen.

Fixed ✅

One last suggestion: we could maybe stop using maximisedContent for the currency and network settings in the Settings modal.

Done ✅

I notice a jump down when swiping the Transaction modal up to expand. As if it first quickly wants to minimise before expanding. When swiping down to minimise, it's the contrary. It seems to first expand then minimize.

This bug already exists in the base branch (you can test it by setting a customMinHeight={600} to the Transaction modal. I think I know what the reason is:

This is the code of handling the layout change of the modal:

Gesture.Pan()
        .onStart((e) => {
          offsetY.value = modalHeight.value
        })
        .onChange((e) => {
          if (position.value !== 'closing') {
            modalHeight.value = offsetY.value + e.translationY
          }
        })
        .onEnd(() => {
          const shouldMinimise = position.value === 'maximised' && -modalHeight.value < dimensions.height - DRAG_BUFFER

          const shouldMaximise =
            canMaximize.value && position.value === 'minimised' && -modalHeight.value > minHeight.value + DRAG_BUFFER

          const shouldClose =
            ['minimised', 'closing'].includes(position.value) && -modalHeight.value < minHeight.value - DRAG_BUFFER

          if (shouldMaximise) {
            handleMaximize()
          } else if (shouldMinimise) {
            scrollableContent ? handleClose() : handleMinimize()
          } else if (shouldClose) {
            handleClose()
          } else {
            modalHeight.value =
              position.value === 'maximised'
                ? withSpring(-maxHeight, springConfig)
                : withSpring(-minHeight.value, springConfig)
          }
        }),

When the user lifts their finger the onEnd gets triggered. However, until the code of onEnd runs to decide what should happen (maximization, minimization, etc), for a split of a second the modal tries to bring itself back to its previous position, but then one of the handleMaximize, handleMinimize gets called. The solution I think is that the decision for maximizing/minimizing should be taken in the onChange callback and not in the onEnd. The onEnd should only apply the decision. WDYT?

Since this is not introduced in this PR, would you mind tackling it in a follow-up?

Copy link
Member

@mvaivre mvaivre left a comment

Choose a reason for hiding this comment

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

That's perfect. Let's merge this!

@nop33 nop33 merged commit 8460983 into UI-refinements Sep 14, 2023
2 checks passed
@nop33 nop33 mentioned this pull request Sep 14, 2023
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants