MudBlazor icon indicating copy to clipboard operation
MudBlazor copied to clipboard

Add seconds to TimePicker

Open DanBiscotti opened this issue 8 months ago • 9 comments

Description

fixes #4644

Hi MudBlazor maintainer(s)! I hope you will consider this PR, I wanted to be able the seconds in the TimePicker component, so thought I would have a go at it myself

I made several changes:

  • Added a copy of the minutes dial widget to allow selecting seconds part of time. This is automatically opened after the minute dial, given the TimeEditMode allows
  • Added 3 additional TimeEditMode
    • HoursMinutes: only hours and minutes are editable
    • MinutesSeconds: only minutes and seconds are editable
    • SecondsOnly: only seconds are editable
  • Added two parameters HoursVisible and SecondsVisible to change whether the hours and seconds are visible in the picker toobar
  • Added parameter SecondSelectionStep which controls the step for seconds in the same way MinuteSelectionStep does for minutes
  • Made a few CSS changes to allow for the extra width of the time format when in static landscape mode

I also fixed a related issue where the OpenTo was not being set correctly based on the TimeEditMode when PickerVariant was set to Static. This was because OnPickerOpenedAsync was never being called to set it correctly so it always opened to the default value for OpenTo which is hours. To fix this I made currentView nullable and set it in OnParametersSet based on the TimeEditMode.

I know it says to have only one issue per PR, but I thought this was so closely related that it might be allowed to be added, but can create a separate PR if this is a hard rule

One thing I didn't add is a way of keyboard navigation for the seconds part, which I notice is there for hours and minutes. I did this mainly because I wasn't 100% sure which path to take. My main thought was to use ctrl+shft+arrow, but don't know if that might clash with other browser keybindings I'm not aware of

How Has This Been Tested?

I tested this using the Docs project and also an extra project I added locally for testing, but I've also added some unit tests

Type of Changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Documentation (fix or improvement to the website or code docs)

Standard usage

Screencast From 2025-04-23 21-06-20

HoursVisible and SecondsVisible

Screencast From 2025-04-23 21-11-07(1)

Layout before and after

Before

image

After

image

Checklist

  • [x] The PR is submitted to the correct branch (dev).
  • [x] My code follows the code style of this project.
  • [x] I've added relevant tests.

DanBiscotti avatar Apr 23 '25 20:04 DanBiscotti

Codecov Report

:x: Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 91.16%. Comparing base (886dcc9) to head (d0bb0f4). :warning: Report is 300 commits behind head on dev.

Files with missing lines Patch % Lines
...lazor/Components/TimePicker/MudTimePicker.razor.cs 88.37% 2 Missing and 3 partials :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #11231      +/-   ##
==========================================
+ Coverage   91.08%   91.16%   +0.08%     
==========================================
  Files         436      466      +30     
  Lines       14143    14477     +334     
  Branches     2731     2805      +74     
==========================================
+ Hits        12882    13198     +316     
- Misses        639      641       +2     
- Partials      622      638      +16     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Apr 23 '25 20:04 codecov[bot]

Please fix the broken tests then tag me and I'll review asap.

Also henon has a few comments that appear to be unanswered/unresolved, can you elaborate on those?

versile2 avatar Jun 02 '25 15:06 versile2

Hi, @henon, @ScarletKuro and @versile2, Thanks for taking a look and for the comments, sorry it took a while to get back. Hopefully the responses make sense. I fixed the tests and made the change suggested to leave make seconds visible = false by default Cheers, Dan

DanBiscotti avatar Jun 02 '25 22:06 DanBiscotti

Hey @versile2, I made the changes to fix the sonarqube issues and also made a change to the unit test.Hopefully I understood your comments correctly. Cheers

DanBiscotti avatar Jun 07 '25 10:06 DanBiscotti

@Anu6is you have time to review before we push it to kuro or dc?

versile2 avatar Jun 07 '25 15:06 versile2

Will take a look, yes

Anu6is avatar Jun 07 '25 15:06 Anu6is

Screencast From 2025-06-09 12-38-17.webm

Hey @Anu6is , thanks I missed that. Should be fixed now. Cheers

DanBiscotti avatar Jun 09 '25 11:06 DanBiscotti