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.

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

Feature/top bar navigation #1560

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

Feature/top bar navigation #1560

wants to merge 17 commits into from

Conversation

araratthehero
Copy link
Contributor

@araratthehero araratthehero commented Apr 18, 2024 •
edited

Description

This PR includes the following changes:

  • Swipe to dismiss Drop-in has been removed
  • Top navigation has been added to all Drop-in screens
  • Bottom sheets do not have rounded corners anymore
  • Going back from actions now dismisses Drop-in, instead of opening it again
  • Gift card confirmation screen now has a title, which shows the payment method name

Checklist

  • Changes are tested manually
  • Link to related issues
  • Add relevant labels to PR

COAND-868

@araratthehero araratthehero requested a review from a team as a code owner April 18, 2024 14:16
@araratthehero araratthehero added the Feature [PRs only] Indicates a new feature addition label Apr 19, 2024
@araratthehero araratthehero marked this pull request as draft April 22, 2024 12:17
@araratthehero araratthehero marked this pull request as ready for review April 22, 2024 13:05
@araratthehero araratthehero force-pushed the feature/top_bar_navigation branch 3 times, most recently from 96bb38b to b0bcb95 Compare April 24, 2024 13:40
jreij
jreij reviewed Apr 24, 2024
View reviewed changes
drop-in/src/main/java/com/adyen/checkout/dropin/internal/ui/CardComponentDialogFragment.kt
@@ -65,6 +59,20 @@ internal class CardComponentDialogFragment : BaseComponentDialogFragment(), Addr
.launchIn(viewLifecycleOwner.lifecycleScope)
}

private fun initToolbar() = with(binding.bottomSheetToolbar) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found an edge case scenario that displays the wrong icon:

  • Load drop-in with only one payment method that can display multiple screens (card + address lookup or BACS)
  • Configure drop-in with setSkipListWhenSinglePaymentMethod(true)
  • Open drop-in and open the second screen
  • A back icon is expected to show but a close icon is actually shown
  • Tapping the icon does go back to the first screen so the tap action works correctly

I don't think we can do this now without adding extra functionality, let's discuss whether it's worth it to handle this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I did not like leaving it like that, so I fixed it. Could you please test it again?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great effort with this! I like the general idea but my main issue with the implementation is that it only works for us internally (drop-in). Merchants using standalone components won't be able to benefit from it (as we're observing component.viewFlow which is internal). It would be great if this became part of some public functionality.

Personally I wouldn't mind if we keep this part out of the scope of this PR as it's getting quite big and all of the solutions are going to be complicated enough to deserve their own PR.

@araratthehero araratthehero force-pushed the feature/top_bar_navigation branch 4 times, most recently from cd11e33 to 8439e6a Compare April 29, 2024 07:19
ozgur00
ozgur00 reviewed Apr 29, 2024
View reviewed changes
bacs/src/main/java/com/adyen/checkout/bacs/internal/ui/DefaultBacsDirectDebitDelegate.kt Outdated
override fun handleBackPress(): Boolean {
val isConfirmationMode = _componentStateFlow.value.mode == BacsDirectDebitMode.CONFIRMATION
return if (isConfirmationMode) {
return if (isModeConfirmation()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a small one: i think we should call canHandleBackPress() here to check instead of isModeConfirmation() directly then we know we will handle back press if we can, then if we want to know how we decide that we can always go see the canHandleBackPress() details.

ozgur00
ozgur00 reviewed Apr 29, 2024
View reviewed changes
card/src/main/java/com/adyen/checkout/card/internal/ui/DefaultCardDelegate.kt Outdated
override fun handleBackPress(): Boolean {
return if (_viewFlow.value == CardComponentViewType.AddressLookup) {
return if (isViewTypeAddressLookup()) {
Copy link
Contributor

@ozgur00 ozgur00 Apr 29, 2024 •
edited

Choose a reason for hiding this comment

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

i'd use the same approach here as well

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Apr 30, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
25.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozgur00 ozgur00 ozgur00 left review comments

@jreij jreij jreij left review comments

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

Assignees
No one assigned
Labels
Feature [PRs only] Indicates a new feature addition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@araratthehero @ozgur00 @jreij