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

Fix MiniCart Interaction Blocking #2547

Merged
merged 7 commits into from Jul 15, 2020
Merged

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Jul 10, 2020

Description

When closed, the MiniCart was blocking interactions on the page behind it.
This PR moves the MiniCart offscreen when it is closed to avoid this behavior.

Alternative solutions:

  • We purposefully don't want to unmount / remount it.
  • Since we're animating visibility we can't switch to using display either because CSS can't animate the display property

Related Issue

Closes PWA-743 / #2546 .

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Go to a Product Detail Page (PDP)
  2. Open the MiniCart
  3. Take note of page elements that it covers (color swatches most likely)
  4. Close the MiniCart
  5. Verify that you can interact with the elements an open MiniCart covers up

Screenshots / Screen Captures (if appropriate)

Screen Shot 2020-07-10 at 12 31 00 PM

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have updated the documentation accordingly, if necessary.

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Jul 10, 2020
@@ -11,6 +11,7 @@
visibility: hidden;

/* It animates to being closed. */
transform: scale(0);
transition-duration: 192ms;
transition-timing-function: var(--venia-global-anim-out);
transition-property: opacity, visibility;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that transform is purposefully omitted from the transition-property: we don't want to animate the growing and shrinking.

Copy link
Contributor

@jimbo jimbo Jul 10, 2020

Choose a reason for hiding this comment

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

Wasn't there supposed to be a little bit of translation going on here, though? I'd swear we had something like transform: translate3d(0, -0.5rem, 0).

edit: the translation I'm thinking of is on a child element, not this one, so ignore the above.

Anyway, generally I'd like to leave transform on the table for animation without known bugs waiting to happen if it's used. Could we just use positioning (left: 100vw) or something?

@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Jul 10, 2020
sirugh
sirugh previously approved these changes Jul 10, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jul 10, 2020

Fails
🚫 A version label is required. A maintainer must add one.
Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-743.

Generated by 🚫 dangerJS against 14fa251

@@ -11,6 +11,7 @@
visibility: hidden;

/* It animates to being closed. */
transform: scale(0);
transition-duration: 192ms;
transition-timing-function: var(--venia-global-anim-out);
transition-property: opacity, visibility;
Copy link
Contributor

@jimbo jimbo Jul 10, 2020

Choose a reason for hiding this comment

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

Wasn't there supposed to be a little bit of translation going on here, though? I'd swear we had something like transform: translate3d(0, -0.5rem, 0).

edit: the translation I'm thinking of is on a child element, not this one, so ignore the above.

Anyway, generally I'd like to leave transform on the table for animation without known bugs waiting to happen if it's used. Could we just use positioning (left: 100vw) or something?

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jul 10, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2547.pwa-venia.com : LH Performance Expected 0.85 Actual 0.52, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 83.333333333333
https://pr-2547.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.36, LH Best Practices Expected 1 Actual 0.92
https://pr-2547.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.41, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 65 Actual 50

revanth0212
revanth0212 previously approved these changes Jul 13, 2020
jimbo
jimbo previously approved these changes Jul 13, 2020
@dpatil-magento
Copy link
Contributor

@supernova-at MFTF test failed again on one scenario. Able to reproduce locally too.

  1. Set browser width to 1280px.
  2. Add 2 configurable products to cart
  3. Go to Cart page and observe Proceed to Checkout button.

Issue - Middle portion of button is not clickable because mini cart image swatch overlapping it mostly.

Image from Gyazo

@supernova-at supernova-at dismissed stale reviews from jimbo and revanth0212 via 59485e7 July 14, 2020 17:45
revanth0212
revanth0212 previously approved these changes Jul 14, 2020
@@ -49,7 +49,7 @@
overflow: hidden;
padding: 1rem;
position: fixed;
right: 0.5rem;
right: -100vw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this doesn't feel like the solution. I think the problem here is that both .root and .contents have position: fixed, which is relative to the viewport. The latter should probably have position: absolute, so that it's relative to the former. That way, when the parent moves offscreen, it'll take the child with it.

.root
  position: fixed

.contents
  position: absolute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks. Resolved in c9812fd.

@sirugh sirugh self-requested a review July 15, 2020 18:21
@dpatil-magento dpatil-magento merged commit 935141a into develop Jul 15, 2020
@dpatil-magento dpatil-magento deleted the supernova/743_minicart_block branch July 15, 2020 19:10
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants