-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: toggle transaction details #2049
Conversation
This is nearly done, I just need to make it accessible then I'll mark it ready for review! |
This should be ready to go! @thomasguillot I've included you for a design review just to make sure things look as expected (especially with how the animation feels!). |
I've slightly tweaked the transitions. 797c458 My main worry is how the scroll behaves. It's quite bouncy. I wonder if the user clicks this they are automatically scrolled down to the |
I think the bounciness will be fixed by this PR: #2052 There's currently a resize listener on the iframe element that attempts to scroll to to the top of the content whenever the iframe resizes. I think the intention is to scroll to the top of the page when transitioning between modal checkout screens, but it ends up getting triggered by anything that could cause a resize, including things like accordions. |
Thanks @thomasguillot and @dkoo! #2052 has been merged, so I've updated this PR from trunk and it's definitely helped with the weird jerkiness -- in my testing, the modal's scroll position is staying put as you toggle open and closed the transaction details, and toggle on and off covering fees. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Great work @laurelfulford!
Hey @laurelfulford, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
# [4.7.0-alpha.1](v4.6.0...v4.7.0-alpha.1) (2025-02-20) ### Bug Fixes * **carousel:** avoid editor crash on empty ([#2058](#2058)) ([c316801](c316801)) * **modal-checkout:** better iframe sizing ([#2052](#2052)) ([aa308f2](aa308f2)) * **recaptcha:** use clone of #place_order button to trigger checkout ([#2028](#2028)) ([46eb8b5](46eb8b5)), closes [#2030](#2030) [#2030](#2030) ### Features * add styles to fix Braintree modal appearance ([#2036](#2036)) ([9ab2c62](9ab2c62)) * add toggle for transaction details ([#2049](#2049)) ([d254aca](d254aca)) * **carousel:** rename block and reorganise settings ([#1962](#1962)) ([9905717](9905717)) * update blocks with new brand ([#2050](#2050)) ([2711302](2711302))
🎉 This PR is included in version 4.7.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
This PR makes the Transaction Details section of the Modal Checkout collapsed by default, and makes it toggle opened/closed.
I originally tried to approach this within the Newspack UI code (Automattic/newspack-plugin#3734 and Automattic/newspack-plugin#3734), but the way we destroy/re-add this table made it hard to get that approach playing nicely and not re-collapsing/requiring a bit of a rewrite.
This approach is strictly tied to the markup in the modal checkout, but let me know if the original approach would've been better!
See 1200550061930446-as-1208982074785832
How to test the changes in this Pull Request:
npm run build
.aria-expanded
set to false, and the attributearia-controls
set to the ID that holds the transaction details table (order_review_content
).aria-expanded
attribute has switched totrue
.aria-expanded
attribute on the heading stays astrue
.aria-expanded
attribute remaintrue
.aria-expanded
attribute remains false.aria-expanded
set totrue
.aria-expanded
attribute is stilltrue
.aria-expanded
attribute is stilltrue
.aria-expanded
attribute remains false.Other information: