Skip to content

Conversation

BenjaminMichaelis
Copy link
Contributor

@BenjaminMichaelis BenjaminMichaelis commented Jan 21, 2022

Description

Added a speed variable to control the speed of "typing". On demo it is called "Print Speed" as shown in the screenshot. Default value is 10 which is equivalent to the previous static value of 5 seconds.

Fixes #12

Type of change

  • Bug fix (added a non-breaking change which fixes an issue)
  • New feature (added a non-breaking change which adds functionality)
  • Updated documentation (updated the readme, templates, or other repo files)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Spent half an hour trying to set up composer on linux and windows and didn't get it working. So manually tested with live deployment here on Heroku

  • Ran tests with composer test
  • Added or updated test cases to test new features

Checklist:

  • I have checked to make sure no other pull requests are open for this issue
  • The code is properly formatted and is consistent with the existing code style
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Screenshots

Readme print speed

@DenverCoder1 DenverCoder1 self-requested a review January 21, 2022 11:12
Copy link
Owner

@DenverCoder1 DenverCoder1 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 great so far.

I think it would be more intuitive to do the duration way instead of speed, though. If using speed without taking into account the width, the same speed will be slow on narrow images and fast on wide images. If duration is used, it is more intuitive since it makes sense regardless of the width. Feel free to let me know what you think. :)

@BenjaminMichaelis
Copy link
Contributor Author

@DenverCoder1 I don't mind changing it to duration, I assume instead of doing (50/speed) we would then be able to just echo $duration s and it would just work theoretically which is an easy change to make unless there is more to it than that that I am missing as I am just thinking about it. It might be more intuitive for time you are right.

I think an improvement to this even would be switching the dur to use milliseconds instead of seconds which I think can only be ints from lightly reading over the documentation in which case I think I will change speed to duration everywhere, and then in dur=echo part I will do $dur*1000 to get it into milliseconds which will allow for more fine control like 5.5 seconds or 3.23 seconds.

Thoughts?

@DenverCoder1
Copy link
Owner

DenverCoder1 commented Jan 21, 2022

Yeah, that sounds good.

If using duration as parameter, I think all you'd need is just to echo the duration instead of (50/speed) like you said.

I think it would be fine to even have the duration parameter be in milliseconds (default 5000) and just to echo $duration with ms.

Either way is fine.

@BenjaminMichaelis
Copy link
Contributor Author

BenjaminMichaelis commented Jan 21, 2022

Updated with the duration variable. Also updated the tests so that they should be passing now (still didn't get composer, but from what I can tell should be good)

Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Looks great 👍

I've tested it and it seems to work. I have just a few minor suggestions

@BenjaminMichaelis
Copy link
Contributor Author

Thanks for the clarifications!

Copy link
Owner

@DenverCoder1 DenverCoder1 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution 🎉

@DenverCoder1 DenverCoder1 changed the title Add speed control variable feat: Add speed control variable Jan 21, 2022
@DenverCoder1 DenverCoder1 merged commit c56ed44 into DenverCoder1:main Jan 21, 2022
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.

Add speed parameter
2 participants