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

UI redesign part3 #858

Merged
merged 45 commits into from
Oct 31, 2024
Merged

UI redesign part3 #858

merged 45 commits into from
Oct 31, 2024

Conversation

mvaivre
Copy link
Member

@mvaivre mvaivre commented Sep 27, 2024

In this PR:

  • Fix Dashboard bugs (amount not appearing sometimes)
  • Migrate all modals to the new system. => Just lacking ONE... WalletConnectSessionRequestModal.
  • Refine the new loading / landing UI

Copy link

changeset-bot bot commented Sep 27, 2024

⚠️ No Changeset found

Latest commit: e0a2948

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mvaivre mvaivre force-pushed the UI-redesign-part3 branch 10 times, most recently from 74af13d to c45fe76 Compare October 2, 2024 09:10
@mvaivre mvaivre changed the base branch from UI-redesign-part2 to banxa-next October 3, 2024 07:43
@nop33
Copy link
Member

nop33 commented Oct 9, 2024

Maybe it's time to break this PR in smaller chunks again? :/ It seems to be growing a lot. Maybe I can start reviewing and you can work on part 4?

Supports the same functionalities as the original one.
TODO: I couldn't use animated scroll utility function anymore, and I feel like the drag gesture is less smooth. Test it.
This will make reusibility way easier (cf. next commit)
UX needs to be tested (should we close the first modal manually?)
Note: The huge diff is due to reorganizing callbacks, not actual changes
Comment on lines +86 to +89
// Using sin allows to smoothen movements (espacially when phon is upside down)
const sinRoll = useDerivedValue(() => Math.sin(gyroscope.sensor.value.roll))
const sinPitch = useDerivedValue(() => Math.sin(gyroscope.sensor.value.pitch))

Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents the weird movements we've seen before

</ButtonsRow>
</ScreenSection>
</ModalContent>
<BottomModal modalId={id}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment about the whole PR: every time I've wrapped a modal in BottomModal, the whole component is marked as changed, but it's not.

What you should take a look at it how I extracted the callbacks, like handleConsolidate above. The main difference is that the modals are now responsible for closing themselves once the callbacks are executed. Before, it was the parents which were handling this.

}

const Screen = ({ children, headerOptions, ...props }: ScreenProps) => {
const Screen = ({ children, headerOptions, safeAreaPadding, usesKeyboard, ...props }: ScreenProps) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the same functionalities as ScrollScreen. We used to use scroll screens when not actually needed. I've replaced some of them with simple non-scrollable screens.

}, [allConfirmedTransactionsLoaded, dispatch, isLoading])

return (
<FlashList
Copy link
Member Author

Choose a reason for hiding this comment

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

It was using a FlatList before.

handleClose,
contentScrollHandlers,
isScrollable
} = useBottomModalState({
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted the animation and gesture logic in a hook, allowing reusability in BottomModalFlashList.


export default BottomModalFlashList

const KeyboardAvoidingViewStyled = styled(KeyboardAvoidingView)`
Copy link
Member Author

Choose a reason for hiding this comment

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

Styles are the same as in BottomModal for now. But it may change in the future - I want to refactor the modals header & title. The implementation will probably not be the same in both BottomModal variants.

@@ -38,6 +38,8 @@ export interface ModalFlatListContentProps<T> extends ModalContentBaseProps, Fla

const scrollDefaultProps = { bounces: false, scrollEventThrottle: 16 }

// TODO: DELETE THIS COMPONENT ONCE BOTTOM MODAL IS REFACTORED
Copy link
Member Author

Choose a reason for hiding this comment

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

We will actually keep ModalContent probably, but the file can be cleaned up a lot.

Comment on lines +157 to +185
const contentScrollHandlers: ContentScrollHandlers = {
onScroll: (e) => {
contentScrollY.value = e.nativeEvent.contentOffset.y

if (!isContentDragged.value) return

if (contentScrollY.value <= 0) {
// Move the modal
if (contentScrollY.value < previousContentScrollY.value) {
const newModalHeightValue = modalHeight.value - contentScrollY.value
modalHeightDelta.value = modalHeight.value - newModalHeightValue
modalHeight.value = newModalHeightValue
}
} else if (-modalHeight.value < maxHeight) {
handleMaximize()
}
previousContentScrollY.value = contentScrollY.value
},
onScrollBeginDrag: () => {
isContentDragged.value = true
},
onScrollEndDrag: () => {
isContentDragged.value = false

if (modalHeightDelta.value < -1) {
handleClose()
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

So, that was a pain in the 🍑
It's not perfect yet, there are some glitches when the use scrolls down the content a bit, then go back up again. But for closing modals quickly it works well.

Copy link
Member Author

Choose a reason for hiding this comment

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

This diff highlight the main limitation with the new way to open modals. Because we need to pass callbacks in openModal, we need to take into account where openModal is called. All needed callbacks must be declared before the function is used, which can be a little tricky. Also, more useCallback hooks are now required.

@mvaivre mvaivre requested a review from nop33 October 11, 2024 09:19
@mvaivre mvaivre marked this pull request as ready for review October 11, 2024 09:19
@@ -31,13 +35,17 @@ type NFTListScreenProps = StackScreenProps<InWalletTabsParamList, 'NFTListScreen
const NFTListScreen = ({ navigation }: NFTListScreenProps) => {
const { t } = useTranslation()
const { screenScrollY, screenScrollHandler } = useScreenScrollHandler()
const listRef = useRef<FlashList<NFT>>(null)
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason this ref is needed?

apps/mobile-wallet/package.json Show resolved Hide resolved
Copy link
Member

@nop33 nop33 left a comment

Choose a reason for hiding this comment

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

I went through all files. The GH UI is very laggy due to the amount of them 😆

I will do some more functional tests once I get some feedback on the review 👍

<View style={{ backgroundColor: 'black', flex: 1 }}>
<AnimatedCirclesBackground isAnimated />
</View>
<Modal visible={!!lastUsedWalletId && biometricsRequiredForAppAccess && !isWalletUnlocked} animationType="fade">
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I remember that changing it to fade creates a weird artefact but I don't remember exactly how. I need to do a functional test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any artefact in the next PR 🤞

@mvaivre mvaivre merged commit 4d5201c into banxa-next Oct 31, 2024
4 of 5 checks passed
@nop33 nop33 deleted the UI-redesign-part3 branch December 11, 2024 17:04
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