Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Hide Accordion closed Sections instead of unmounting them #2149

Merged
merged 10 commits into from
Feb 11, 2020
Merged

Hide Accordion closed Sections instead of unmounting them #2149

merged 10 commits into from
Feb 11, 2020

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Feb 7, 2020 •
edited
Loading

Description

This PR hides accordion section contents that aren't open instead of not rendering them at all.
This saves components from mounting / unmounting on section open changes, but may overfetch (if a section is never opened, its contents may still make network calls).

This PR also sets all price adjustment accordion sections to closed initially, which is the intended UX.

Related Issue

Closes PWA-360.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Go to the /cart page
  2. Inspect element on the price adjustments Accordion
  3. See that the contents of the sections that are closed still appear in the DOM, they're just hidden from the user

Screenshots / Screen Captures (if appropriate)

Checklist

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

@supernova-at supernova-at added the version: Patch This changeset includes backwards compatible bug fixes. label Feb 7, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 7, 2020 •
edited
Loading

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-360.

Generated by 🚫 dangerJS against 637fbbd

@sirugh
Copy link
Contributor

sirugh commented Feb 7, 2020

<3 Perfect. Fix the tests and I'll approve :D

@supernova-at supernova-at changed the title Supernova/360 accordion Hide Accordion closed Sections instead of unmounting them Feb 7, 2020
revanth0212
revanth0212 previously approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@revanth0212 revanth0212 left a comment

Choose a reason for hiding this comment

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

Nice optimization.

schensley
schensley previously approved these changes Feb 7, 2020
View reviewed changes
Copy link

@schensley schensley left a comment

Choose a reason for hiding this comment

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

UX approved.

@supernova-at supernova-at dismissed stale reviews from schensley and revanth0212 via 91ef9f3 February 10, 2020 16:00
@supernova-at
Copy link
Contributor Author

PR Updated:

  • Fixes merge conflicts

supernova-at
supernova-at commented Feb 10, 2020
View reviewed changes
packages/venia-ui/lib/components/CartPage/PriceAdjustments/__tests__/priceAdjustments.spec.js
jest.mock('../giftCardSection', () => 'GiftCardSection');
jest.mock('../GiftOptions', () => 'GiftOptions');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that the children get rendered regardless of whether their section is open or not, we have to mock them all.

@supernova-at
Copy link
Contributor Author

PR Updated:

  • Updates priceAdjustments unit tests and snapshots

@dpatil-magento
Copy link
Contributor

@supernova-at Not sure how its related but as demo'ed happens only with this PR, works on develop

When try to sign-out from cart page, sign-out does not work and console has this error.

vendors.e2bb76dc931329fff8e3.js:1059 Uncaught (in promise) Error: Network error: Cannot read property 'include_gift_receipt' of undefined at new ApolloError (vendors.e2bb76dc931329fff8e3.js:1059) at vendors.e2bb76dc931329fff8e3.js:1142 at async Promise.all (index 6)

@supernova-at
Copy link
Contributor Author

supernova-at commented Feb 11, 2020 •
edited
Loading

Hey @dpatil-magento I was able to reproduce that issue on develop so I have created a separate issue (PWA-368) for it and this one can be merged (since it didn't introduce the issue).

dpatil-magento
dpatil-magento approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@dpatil-magento dpatil-magento left a comment

Choose a reason for hiding this comment

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

Approved latest commits.

@dpatil-magento dpatil-magento merged commit dbda97e into develop Feb 11, 2020
@dpatil-magento dpatil-magento deleted the supernova/360_accordion branch February 11, 2020 15:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dpatil-magento dpatil-magento dpatil-magento approved these changes

@schensley schensley schensley left review comments

@revanth0212 revanth0212 revanth0212 left review comments

Assignees

@supernova-at supernova-at

@schensley schensley

@revanth0212 revanth0212

@dpatil-magento dpatil-magento

Labels
pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@supernova-at @PWAStudioBot @sirugh @dpatil-magento @schensley @revanth0212