-
-
Notifications
You must be signed in to change notification settings - Fork 46
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 option to display the full time (HH:mm:ss) in the timer view #988
Conversation
Important Review SkippedReview was skipped due to path filters Files ignored due to path filters (3)
You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
The change itself looks good to me, but I am a bit confused with the setup.
Maybe I misunderstood something?
apps/client/src/common/components/view-params-editor/constants.ts
Outdated
Show resolved
Hide resolved
apps/client/src/common/components/view-params-editor/constants.ts
Outdated
Show resolved
Hide resolved
275786c
to
c56dd86
Compare
c56dd86
to
8b6d0ec
Compare
Ok so i fixed the typos, also inverted the logic to keep the default setting as it use to be to be. I think it's fully functional now. PS: I'll look forward to setup a proper environment to deal with node (actually using vim to edit files is pretty limited) |
@@ -30,6 +30,14 @@ const hideTimerSeconds: ParamField = { | |||
defaultValue: false, | |||
}; | |||
|
|||
const showLeadingZeros: ParamField = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all other options are for hiding custom behaviour, should we keep it here as well? I guess we can just default it to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't know. Usually I prefer keeping the original behavior and use an option to change it.
User updating the app will probably be surprised that the default format of the timer has changed ?
But if you prefer i can reverse the condition to make it the new default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the variable was to hide, and the default was on, the users wouldnt see a difference.
My suggestion was with the inclination that all the other options are to hide things, so this one is different.
But I guess you may be correct in the idea that it doesnt matter if it is hide / show, what we are doing is changing the default behaviour. In which case your change makes more sense than my proposal. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the text and the default value of the option should be nice to have all the option text to do a "hide function" but i struggle with the boolean parser here:
https://github.com/cpvalente/ontime/blob/master/apps/client/src/features/viewers/common/viewUtils.ts#L32-36
When the parameter is absent from the parameters part in url it will be parsed to "false" thus breaking the default value.
I don't know how to fix this so that's why I inverted the logic from "hide" to "show"
Thank you @thelan , left some small suggestions and questions for you. Setting up an environment should be pretty straightforward, you can also use GitHub codespaces in your fork. |
Co-authored-by: Carlos Valente <[email protected]>
Thank you @thelan , apologies it took so long. |
Hello,
Its just a small patch to add an view option to enable the timer to display the full-clock instead of being hardcoded to remove leading zeros.
Let me know if i need to change or optimize things