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

Added civilian time option for timeInput function #20

Merged
merged 9 commits into from
Apr 14, 2024

Conversation

nick-youngblut
Copy link
Contributor

I've added an option to use civilian (non-military) time (#12). Feel free to change the parameter name to twelve.hour or another name. I don't have much experience with editing the javascript section of shiny widgets, but it appears to work correctly (see attached).

I added a width parameter (#19).

I've also added documentation for the utility functions and also examples for using the civilian time feature.

Screenshot 2024-03-10 at 9 15 21 AM Screenshot 2024-03-10 at 9 15 30 AM Screenshot 2024-03-10 at 9 15 41 AM

@burgerga
Copy link
Owner

Thanks for the pull request! It's quite a substantive change, so I need some time to review it. I will get back to you ;)

@burgerga
Copy link
Owner

Question: does the current time button in the example app work for you (it appears from the screenshots that it does)? For me, that button does update the inputs, but the text on the right doesn't...

@nick-youngblut
Copy link
Contributor Author

does the current time button in the example app work for you

The text was updating for me... but now it is not, and I don't know why 🫤. Any ideas?

@nick-youngblut
Copy link
Contributor Author

The issue was due to including input in the change.timeInputBinding binding:

    $(el).on("change.timeInputBinding", "input", function(e) {
      correctInputValue(e.target);
      callback();
    });

changed to:

    $(el).on("change.timeInputBinding", function(e) {
      correctInputValue(e.target);
      callback();
    });

@nick-youngblut
Copy link
Contributor Author

nick-youngblut commented Mar 17, 2024

@burgerga you probably have noticed that changing the AM/PM value in the UI element results in a blank value shown instead of "AM" or "PM" (see attached). It appears to be an issue with setValue and/or subscribe, but I can't figure out the issue. Any ideas?

I should note that textOutput("time_output3") changes very briefly when the AM/PM value is changed to the alternative value (e.g., AM --> PM), but then the value switches back to the starting value (e.g., AM --> PM --> AM; all very quickly).

Screenshot 2024-03-17 at 11 24 43 AM Screenshot 2024-03-17 at 11 24 48 AM

@nick-youngblut
Copy link
Contributor Author

@burgerga I figured out the issue: correctInputValue was converting the AM/PM values to 0. I've added an early return to the function if the class is shinytime-civilian.

@nick-youngblut
Copy link
Contributor Author

@burgerga I just wanted to check in on this PR. Do you have any concerns prior to merging?

- Remove documentation for params already documented in shiny::textInput
- Fix line order for minute.steps documentation
- Add titles to utils documentation
@burgerga
Copy link
Owner

burgerga commented Apr 5, 2024

Hi @nick-youngblut, sorry for the late reply, hectic period :/
I am almost done with the pull request, it looks good, great work! I'm making some additional changes such that the specified width applies to the whole shinyTime input group, that way you can align it with other elements.

@burgerga
Copy link
Owner

burgerga commented Apr 5, 2024

By the way, I want to credit this significant contribution, do you object to being listed as package author (see ?person)? (and is your orcid https://orcid.org/0000-0002-7424-5276?)

@nick-youngblut
Copy link
Contributor Author

@burgerga thanks for the update! I'd be happy to be listed as an author. Yep, that's my ORCID. Thanks!

@burgerga
Copy link
Owner

Hi @nick-youngblut, can you have a look?
I added the non-exported shinyTime:::shinyTimeDebug() function to preview behavior at different widths in bslib.

@nick-youngblut
Copy link
Contributor Author

Looks great! Thanks again for adding me as an author.

@burgerga burgerga merged commit 1e7d35f into burgerga:master Apr 14, 2024
1 of 2 checks passed
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