Skip to content

Conversation

@RickyLin
Copy link
Contributor

@RickyLin RickyLin commented Aug 21, 2024

Pull Request

📖 Description

  1. Added OnDoubleClick event to DatePicker component.
  2. Added DoubleClickToToday parameter to DatePicker component.
    2.1 When DoubleClickToToday is true, today's date is set to the date picker after a user double-clicks on the text field of the date picker.
  3. Refactored the way how calendar is closed when clicking on the outside of the date picker.
    1. Currently there's an overlay over the text field which is used to close the calendar when a user clicks outside of the date picker. This leads to two issues:
      1. It's difficult to trigger double-click event on the text field because one click is taken by the overlay?
      2. It makes switching to other components by mouse click not smoothly. I click on date picker and calendar shows, then I click another component but the focus doesn't switch to that component because the click is taken by the overlay to close the calendar.
    2. I tried solving this issue by removing the overlay and leveraging the JavaScript mousedown event on document element to close calendar when clicking on outside of the date picker.

🎫 Issues

The PR was related to the discussion #2524 How can I extend FluentDatePicker to support double-click event on its text field?

👩‍💻 Reviewer Notes

3 functionalities to test:

  1. The OnDoubleClick event should work.
  2. The DoubleClickToToday parameter should work.
  3. When the calendar of date picker is open, click elsewhere of the date picker should close the calendar, meanwhile the element on which the mouse clicks should get focus.

📑 Test Plan

Only DatePicker needs to be tested, it should not affect other components.

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

No follow-up work to this PR.

@RickyLin
Copy link
Contributor Author

@microsoft-github-policy-service agree

@vnbaaij
Copy link
Collaborator

vnbaaij commented Aug 21, 2024

I think everything looks good. Thanks for this!

Can you just add / change one of the examples to show this new behavior? Maybe alter https://www.fluentui-blazor.net/DateTime#defaultdatepicker with a checkbox to enable/disable DoubleClickToToday (and add some explanation in the Description tag on the DateTime page?

@dvoituron
Copy link
Collaborator

I'm not sure that removing the FluentOverlay is a good solution. I prefer to keep as much code as possible in the Blazor/C# part. What's more, it's likely that the FluentOverlay will be improved later to handle other functions such as ESC to close the panel.

On the other hand, using JavaScript in Blazor dissociates the logic known by Blazor and by the client part, which is not always recommended (unless you can't do without it).

So can you separate this PR into two PRs: the first which only manages the new DoubleClick functionality and the second which does the refactoring? I'd like to check the problems you've outlined to see what the best solution is (if possible without JS).

@RickyLin
Copy link
Contributor Author

Hi @dvoituron,
No problem, I'll send 2 PRs as you said.

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.

3 participants