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

feat: hideOnComplete added to allow the timer to stay visible, even after completion! #14

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

liquidg3
Copy link
Contributor

Case:

It was the intuition of the team that the counter would stay at zero unless a completion component was set. This change adds a new feature, which is essentially (hang at zero) while maintaining the old functionality of showing a completion component if one is set.

Notes:

I added a test, kept original ones working. BUT, it did require me to npm install postcss-present-env to get the react example to work. My assumption is that you have it globally installed on your dev machine.

Conclusion:

Great work! Easiest the best looking flip style timer out there!

@liquidg3
Copy link
Contributor Author

Also, lemme know if you don't plan to incorporate this and I'll publish a separate npm module with the functionality, we can't deploy as is!

@sLeeNguyen
Copy link
Owner

@liquidg3 thank you for your PR. I was also thinking of adding this feature in the next release.

By default, the clock will be hidden if children are none (at the current version v1.4.1). It seems that the changes made in your PR have caused an issue with backward compatibility when compared to the previous versions. And I don't want to make a major change just for this feature.

In my opinion, it would be better if we add another boolean property (e.g. hideOnComplete - true by default) to maintain backward compatibility.

What do you think about it? Let me know your opinion, we can discuss to find the best solution.

package.json Outdated Show resolved Hide resolved
@liquidg3 liquidg3 changed the title feat: don't attempt to render children if none are set! feat: hideOnComplete added to allow the timer to stay visible, even after completion! Nov 29, 2023
@liquidg3
Copy link
Contributor Author

@sLeeNguyen - ok, done! Lemme know what you think!

🙌🙌🙌

@liquidg3
Copy link
Contributor Author

Oh, I realized I missed the have a discussion part. We're demoing tomorrow and getting this in would mean I don't have to speak to it disappearing. Thanks so much dude!!

@liquidg3
Copy link
Contributor Author

@sLeeNguyen demoing today. I can publish something new to npm if the timing is bad for you. I tried linking locally and that didn't work, I'll try that before publishing. But, would way prefer to use your component and continue to benefit from any enhancements.

@sLeeNguyen
Copy link
Owner

@liquidg3 sorry for the delay. I'm going to check it now.

@sLeeNguyen
Copy link
Owner

Everything looks fine. Thank you for your contribution

@liquidg3
Copy link
Contributor Author

liquidg3 commented Nov 30, 2023

@sLeeNguyen - is that fail related to yarn cache failing? lemme know if I can help with any failed/checks/etc.

@sLeeNguyen
Copy link
Owner

@sLeeNguyen - is that fail related to yarn cache failing? lemme know if I can help with any failed/checks/etc.

it's ok. Ready to merge now

@sLeeNguyen sLeeNguyen merged commit ec5ace5 into sLeeNguyen:master Nov 30, 2023
3 of 5 checks passed
@liquidg3
Copy link
Contributor Author

@sLeeNguyen woohoo! Thanks so much! As soon as a new version is published I'll give it a shot and if I get a chance film it in action so you can see it!!

@sLeeNguyen
Copy link
Owner

I'm doing local check now. I will inform you when it's done

@sLeeNguyen
Copy link
Owner

@liquidg3 I've just released v1.5.0. Lemme know if there are any problem

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.

2 participants