Skip to content

Fix Delay Attributes #3

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

Merged
merged 3 commits into from
Jun 4, 2021
Merged

Fix Delay Attributes #3

merged 3 commits into from
Jun 4, 2021

Conversation

clraconis
Copy link
Contributor

I found character delay and line delay aren't been set correctly even if I provided a valid value.
I think the bug is caused by a typo in the getter function, the minimum of 0, 2_147_483_647, data-xxx-delay is the only param of Math.max.
So I tried to fix the bug by moving the lower bound out of the calling of Math.min and it seems to work now.
(Please forgive me for my poor English😥)

Fixed the bug that lineDelay and characterDelay aren't parsed correctly
@nholden
Copy link
Contributor

nholden commented Jun 2, 2021

👋 Hi, @hazukitenki! Thanks for this contribution! ✨

Would you mind adding a test for this? I think we'd want one like this except where we specify the data attributes when we create the <typing-effect> element instead of using the defaults.

@clraconis
Copy link
Contributor Author

This is my honor, but I lack the experience of writing tests.
I wrote this test earlier, but I am not sure if it is sufficient for this situation😖. Do we need more test cases to ensure that the delay attribute is always within the valid range?

Copy link
Contributor

@nholden nholden 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 perfect to me, thank you! We should be able to get it merged and included in a new release this week.

Do we need more test cases to ensure that the delay attribute is always within the valid range?

Ideally we'd have some tests for when the data attributes have invalid values, but for the purposes of this PR, the test you wrote is exactly what we needed.

@clraconis
Copy link
Contributor Author

You're welcome. I also benefited a lot from it, thank you! 😀

@koddsson koddsson mentioned this pull request Jun 4, 2021
2 tasks
@koddsson koddsson merged commit aec90cf into github:main Jun 4, 2021
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.

4 participants