AlaskaAirlines / auro-accordion

Custom element that allows users to toggle the display for sections of content
https://auro.alaskaair.com/components/auro/accordion
Apache License 2.0
1 stars 1 forks source link

Add scroll into view feature #37

Closed blackfalcon closed 2 years ago

blackfalcon commented 2 years ago

Alaska Airlines Pull Request

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes: #31

Summary:

This PR moves the responsibility of the scrollToView from the individual accordion element to the group element fixing the timing and placement of the scroll position.

Type of change:

Please delete options that are not relevant.

Checklist:

By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.

Thank you for your submission!
-- Auro Design System Team

jason-capsule42 commented 2 years ago

The change does trigger the the scrolling when the accordion is expanded, however, it only seems to do a portion of the necessary scrolling. When closing the accordion it scrolls the additional amount that was necessary to show all the content

https://user-images.githubusercontent.com/23223137/137483018-124e0c15-7b71-4917-9d30-1c7a7b315beb.mov

.

blackfalcon commented 2 years ago

I will admit that this is not perfect. But the issue that @Eliza-Huang pointed out is common across all accordion components I reviewed. The scrollIntoView feature is a not perfect either and digging around I found countless hacks to make this just a little better. The main issue is that there is no callback for when an animation is completed and the scroll executes.

Moving the event to the group versus the element did make an improvement in UX. On larger devices, this is typically not an issue due to available screen space and smaller content. On mobile devices is where this has the most impact as shown in this gif. Oct-15-2021 09-35-17