Skip to content

Conversation

gksander
Copy link
Contributor

Description

This PR adds a boolean prop animateListItems to the Markdown suite of components that automatically transforms MD list items into Appear > ListItem. This allows a user to provide MD like:

<MarkdownSlide animateListItems>
{`
# Hey world

- One
- Two
- Three
`}
</MarkdownSlide>

and have the list items appear in one at a time.

This addresses issue #968.

Type of Change

Please delete options that are not relevant (including this descriptive text).

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

I have added some Jest/Enzyme tests to test this new prop. I also added an example to the JS example deck that uses this new prop, which can be used to verify that the list items are animating in accordingly.

Comment on lines +39 to +40
expect(wrapper.find('ul')).toHaveLength(1);
expect(wrapper.find(Appear)).toHaveLength(3);
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'd love to also test for the existence of ListItem components, but those only appear as you step through the slide. I wasn't sure if figuring out how to imperatively do that was worth the time. The enzyme tests are already in the realm of "testing implementation details", so this set of assertions felt sufficient.

In the future, we might consider moving to react-testing-library for testing to move away from testing implementation details. My ideal test would be along the lines of:

  • Mount the slide.
  • Check that none of the list items are present.
  • Simulate a click.
  • Check that the first list item is present (searching by text), but not the last two.
  • Simulate two more clicks.
  • Check that all list items are present (searching by text).

Comment on lines 3 to +4
import {
CodePane,
Heading,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were some unused imports in this file. I usually clean this up with an editor action for "optimizing imports" (which clears those out). Looks like it also sorts them by alphabetical order, which is why this diff looks bigger than it should. (In effect, all that happened was CodePan and CodeSpan were removed.)

@gksander gksander requested a review from carloskelly13 March 12, 2021 17:00
Copy link
Contributor

@carloskelly13 carloskelly13 left a comment

Choose a reason for hiding this comment

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

This looks fantastic!

@gksander gksander merged commit 4e9a5ad into main Mar 15, 2021
@gksander gksander deleted the md-list-appear branch March 15, 2021 13:25
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