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

Add d2l-expand-collapse component to create collapsible areas #540

Merged
merged 14 commits into from
May 19, 2020

Conversation

AllanKerr
Copy link
Contributor

This is a componentized version of our d2l-dom-expand-collapse that I'm looking for some input on if we want this added as generic component in core.

Goals

  1. Short-term:
    1. Use this to add expand / collapse support to d2l-alert.
  2. Long-term:
    1. Update d2l-more-less to use this internally.
    2. Replace the 2 LMS usages of the d2l-dom-expand-collapse with this and delete d2l-dom-expand-collapse entirely.

@AllanKerr AllanKerr marked this pull request as draft May 11, 2020 18:14
@AllanKerr AllanKerr force-pushed the akerr/expand-collapse/dev branch from 5367e39 to c7cd86d Compare May 11, 2020 18:33
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
Add aria-expanded to the button in the demo as an example of how to use it.
Remove px from height: 0.
Use the _getContent helper instead of calling querySelector.
Allan Kerr added 5 commits May 12, 2020 15:31
…tioning from display: none or from auto to 0

This was fixed by adding a double requestAnimationFrame wait. The reason for this is that when a Lit property gets updated, the browser
will not immediately perform a style recalc. The next style recalc will happens after the next requestAnimationFrame. If we try to update
styles again in the first requestAnimationFrame block then the first style change from the property change will be overwritten and never
get displayed by the browser causing the transition to be skipped. In the second requestAnimationFrame we can guarantee that the property
style change has taken effect so it is now safe to make our dependent style update.
@AllanKerr AllanKerr marked this pull request as ready for review May 14, 2020 18:01
Copy link
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

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

Really like where this ended up!

components/expand-collapse/README.md Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
components/expand-collapse/expand-collapse.js Outdated Show resolved Hide resolved
<li>Tea</li>
<li>Milk</li>
</ul>
</d2l-expand-collapse>
Copy link
Member

@dlockhart dlockhart May 15, 2020

Choose a reason for hiding this comment

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

Interesting that the blue border still appears even when it's collapsed. It makes sense given it's the internal container that gets a display: none on it, but as a consumer of this I wouldn't have expected it. I can't think of a super clean way to fix this without reflecting that _state property up to the host. Hmm...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think having the border still show up will work well. It aligns with FACE's use of expand collapse where they have a line where the expand collapse section is when collapsed. Implementing it like this means it can be done with an attribute selector:
Capture

It will also make it a lot easier to use with d2l-more-less since that component doesn't fully collapse by default, it leaves 4 ems visibile.

…itial state change so it can start expanded or collapsed
Allan Kerr added 2 commits May 15, 2020 12:27
Add quotes around attribute
Remove separate _getContent helper and inline it
Add link to w3 aria-expanded in README
@AllanKerr AllanKerr force-pushed the akerr/expand-collapse/dev branch from c206ce8 to 3ba6ee7 Compare May 15, 2020 17:20
@AllanKerr AllanKerr merged commit 5d60e00 into master May 19, 2020
@AllanKerr AllanKerr deleted the akerr/expand-collapse/dev branch May 19, 2020 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants