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

Shipping Labels Onboarding: Installation UI #6811

Merged
merged 13 commits into from
Jun 29, 2022

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Jun 24, 2022

Part of: #6587

Description

This PR adds the installation UI, with the animations for the transition from the pre-install step, the changes required some refactorings, as I'm learning about Compose animation APIs, and I found out that the usage of animateEnterExit made the code a bit cleaner.

The additional change is replacing the cancel button with a cross-icon as this was confirmed by the design team.

NB: the installation itself is not implemented yet.

Testing instructions

  1. Make sure your store is in US, and uses USD as the currency, but doesn't have WCShip.
  2. Create an order eligible for shipping label creation (ie: not virtual/downloadable and not eligible for IPP).
  3. Open the order in the app.
  4. Click on the "Get WooCommerce Shipping" button.
  5. Click on "Add extension" to the store.
  6. Click on "Proceed with installation".
  7. Confirm the animation and transition look and work as expected.

You can also run the preview PreviewInstallWCShippingScreen if you want to jump directly into the screen.

Images/gif

Screen.Recording.2022-06-27.at.19.10.16.mov
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Jun 24, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@hichamboushaba hichamboushaba added feature: shipping labels Related to creating, ordering, or printing shipping labels. type: enhancement A request for an enhancement. labels Jun 24, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 24, 2022

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2022

Codecov Report

Merging #6811 (0fbd272) into trunk (754675a) will decrease coverage by 0.26%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##              trunk    #6811      +/-   ##
============================================
- Coverage     44.91%   44.65%   -0.27%     
  Complexity     2966     2966              
============================================
  Files           533      533              
  Lines         29187    29359     +172     
  Branches       3861     3879      +18     
============================================
  Hits          13109    13109              
- Misses        14861    15033     +172     
  Partials       1217     1217              
Impacted Files Coverage Δ
...merce/android/ui/shipping/InstallWCShippingFlow.kt 0.00% <0.00%> (ø)
...android/ui/shipping/InstallWCShippingOnboarding.kt 0.00% <0.00%> (ø)
.../android/ui/shipping/InstallWCShippingViewModel.kt 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 754675a...0fbd272. Read the comment docs.

@hichamboushaba hichamboushaba added this to the 9.6 milestone Jun 27, 2022
@hichamboushaba hichamboushaba marked this pull request as ready for review June 27, 2022 18:13
if (it) 0 else 120
} ?: remember { mutableStateOf(0) }

private fun AnimatedVisibilityScope.PreInstallationContent(viewState: InstallationState.PreInstallation) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Scoping the composables to AnimatedVisibilityScope gives us access to the animateEnterExit modifier, which is quite helpful for composing child animations, and not just relying on the AnimatedContent animation itself.

)
) {
// fill equivalent space as the cross-icon
Spacer(modifier = Modifier.height(dimensionResource(id = R.dimen.major_300)))
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'm using Spacers to account for the space of the contents of other steps, this is needed to have a smooth animation where views don't jump from their positions.
If you know of a better approach, please suggest it 🙏.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, setting the InstallationLoadingIndicator at a fixed margin from the top would the the trick I guess. But then you wouldn't be able to use Spacers with weight for a better distribution of the available space. So I think this approach is perfectly fine and easy to follow with the comment indicating why the extra spacing is needed for each of the cases were you've had to add these.

step.filter { it == Installation }
.first()

// TODO launch plugin installation
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 can here launch the installation then listen to its state to update the step, and since step is saved to the SavedStateHandle, a restoration after a process death for example would restart the installation if it was the step we were in before, which is what we need.

@hichamboushaba hichamboushaba changed the title Shipping Labels Onboarding: Installation UI (part 1) Shipping Labels Onboarding: Installation UI Jun 27, 2022
@JorgeMucientes JorgeMucientes self-assigned this Jun 28, 2022
Comment on lines +340 to +341
val circleColor = colorResource(id = R.color.woo_purple_20)
val progressColor = colorResource(id = R.color.woo_purple_50)
Copy link
Contributor

Choose a reason for hiding this comment

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

Np, but maybe we should add a "semantic" color for these inside values/colors_base.xml instead of referencing directly colors from wc_colors_base.

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'll keep the usage as is for now, as I don't have a good name to name them besides the woo_purple_... 😄.
We can always fix it later.

)
) {
// fill equivalent space as the cross-icon
Spacer(modifier = Modifier.height(dimensionResource(id = R.dimen.major_300)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, setting the InstallationLoadingIndicator at a fixed margin from the top would the the trick I guess. But then you wouldn't be able to use Spacers with weight for a better distribution of the available space. So I think this approach is perfectly fine and easy to follow with the comment indicating why the extra spacing is needed for each of the cases were you've had to add these.

}

@Composable
private fun InstallationInfoLink(onClick: () -> Unit, modifier: Modifier = Modifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Np, the modifier param here is unused. it should be added to the Row container.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, fixed in 619a9b7

import com.woocommerce.android.ui.shipping.InstallWCShippingViewModel.ViewState.InstallationState.PreInstallation

@OptIn(ExperimentalTransitionApi::class)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can remove this. AS states no experimental API is in use here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch too, I removed it, it was a leftover from the previous iteration.

@JorgeMucientes
Copy link
Contributor

Awesome job @hichamboushaba 🙌🏼 This was not an easy animation to achieve. Looks really great!
I left a couple of minor nitpicks, feel free to apply them, other than that ready to 🚢

@peril-woocommerce
Copy link

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@hichamboushaba
Copy link
Member Author

Thanks @JorgeMucientes for the review.

@hichamboushaba hichamboushaba merged commit 806e2df into trunk Jun 29, 2022
@hichamboushaba hichamboushaba deleted the issue/6587-installation-UI branch June 29, 2022 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: shipping labels Related to creating, ordering, or printing shipping labels. type: enhancement A request for an enhancement.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants