Repository navigation
feat(DateTimePicker/Range): improve user experience on mobile - #8663
Open
Tony-ST0754 wants to merge 73 commits into
Open
Tony-ST0754 wants to merge 73 commits into
Tony-ST0754 wants to merge 73 commits into
Conversation
feat(DateTimePicker):添加组件在移动端弹窗显示
revert:remove UseNativeMobileComponents parameter
feat:给 Modal 模态窗添加 Zindex 参数，给用户来控制弹窗层级
refactor:取消 js 中控制弹窗层级的逻辑 feat:增加 ModalZIndex 参数，由用户控制移动端弹窗逻辑
refacotr:重构移动端弹窗侧边栏显示，更多/更少 refacotr:将DatePickerBody.razor中的内容分段提取独立，以让主体Razor代码更清晰
test:补充单元测试
Contributor
Reviewer's GuideThe PR keeps DateTimePicker’s desktop popover unchanged while detecting mobile breakpoints and routing the picker into a configurable Bootstrap modal. It adds mobile-specific lifecycle and JavaScript panel relocation, responsive sidebar collapse/toggle behavior, modal footer actions, styling/localization, and comprehensive unit coverage. Sequence diagram for mobile DateTimePicker modal flowsequenceDiagram
actor User
participant DateTimePicker
participant DatePickerBody
participant JavaScript
participant Modal
User->>JavaScript: click .dropdown-toggle
JavaScript->>DateTimePicker: TriggerShowModalCallback()
DateTimePicker->>Modal: Show()
DateTimePicker->>JavaScript: showModal(Id, modal.Id)
JavaScript->>JavaScript: move picker panel into modal body
JavaScript->>JavaScript: initSidebar()
User->>DatePickerBody: select date or action
DatePickerBody->>DateTimePicker: OnConfirm or OnClear
DateTimePicker->>Modal: Close()
Modal-->>JavaScript: hidden.bs.modal
JavaScript->>JavaScript: restorePanel()
JavaScript-->>DateTimePicker: TriggerHideCallback()
Flow diagram for mobile sidebar collapse and expansionflowchart TD
Open[Mobile picker opens] --> Measure[Measure sidebar content]
Measure --> Overflow{Multiple rows?}
Overflow -->|No| ShowOne[Show sidebar row]
Overflow -->|Yes| Collapse[Show first row and enable toggle]
Collapse --> Toggle[Click display more]
Toggle --> Expand[Show all sidebar rows]
Expand --> ToggleLess[Click display less]
ToggleLess --> Collapse
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your PR, @Tony-ST0754. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8663 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 776 776
Lines 35248 35424 +176
==========================================
+ Hits 35248 35424 +176
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js" line_range="18-22" />
<code_context>
const createPopover = (el, invoke, options) => {
+ EventHandler.on(el, 'click', '.dropdown-toggle', e => {
+ const mobile = el.getAttribute('data-bb-mobile');
+ if (mobile !== 'false') {
+ e.preventDefault();
+ e.stopImmediatePropagation();
+ if (mobile === 'true') {
+ requestModal(Data.get(el.id));
+ }
</code_context>
<issue_to_address>
**First picker click does nothing**
When the picker is clicked before the initial responsive breakpoint callback sets `data-bb-mobile`, the click handler prevents the default action and stops propagation while the value is not `false`, but requests a modal only when it is `true`; the first click therefore opens neither the modal nor the popover.
Handle the unset mobile state so an early click opens the appropriate picker presentation without suppressing the desktop popover.
Also at `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs:386`.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js" line_range="18-23" />
<code_context>
const createPopover = (el, invoke, options) => {
+ EventHandler.on(el, 'click', '.dropdown-toggle', e => {
+ const mobile = el.getAttribute('data-bb-mobile');
+ if (mobile !== 'false') {
+ e.preventDefault();
+ e.stopImmediatePropagation();
+ if (mobile === 'true') {
+ requestModal(Data.get(el.id));
+ }
+ }
</code_context>
<issue_to_address>
**Date range picker stops opening**
When a caller uses `DateTimeRange` with this shared picker module, `createPopover` now cancels clicks whenever `data-bb-mobile` is not exactly `false`, but `DateTimeRange` uses this shared module without that attribute and has no modal-opening callback; its dropdown therefore never opens when clicked.
Restrict the mobile click interception to DateTimePicker instances that opt into the mobile behavior, or provide an explicit non-mobile default for DateTimeRange.
</issue_to_address>
### Comment 3
<location path="src/BootstrapBlazor/Components/Modal/Modal.razor" line_range="6" />
<code_context>
<BootstrapBlazorRootContent>
- <div @attributes="@AdditionalAttributes" class="@ClassString" tabindex="-1" role="dialog" id="@Id"
+ <div @attributes="@AdditionalAttributes" class="@ClassString" style="@StyleString" tabindex="-1" role="dialog" id="@Id"
data-bs-backdrop="@Backdrop" data-bs-keyboard="@KeyboardString">
<CascadingValue Value="this" IsFixed="true">
</code_context>
<issue_to_address>
**Caller modal styles disappear**
When a caller supplies a `style` attribute through `AdditionalAttributes`, the explicit `style` attribute uses `StyleString`, which contains only the generated z-index declaration and does not include the caller's style; the caller's inline styles are missing from the rendered modal.
Merge the caller-provided style from `AdditionalAttributes` with the generated z-index style.
Also at `src/BootstrapBlazor/Components/Modal/Modal.razor.cs:30-32`, `src/BootstrapBlazor/Components/Modal/Modal.razor:7`.
</issue_to_address>
### Comment 4
<location path="src/BootstrapBlazor/Components/Modal/Modal.razor.cs" line_range="31" />
<code_context>
.Build();
+ private string? StyleString => CssBuilder.Default()
+ .AddClass($"--bs-modal-zindex:{ZIndex};", ZIndex.HasValue)
+ .Build();
+
</code_context>
<issue_to_address>
**Backdrop covers low-z-index modal**
When `ZIndex` is set below Bootstrap's default backdrop z-index of 1050, `ZIndex` changes only the modal's stacking level, leaving the backdrop at its default level, so the backdrop covers the dialog and makes it inaccessible.
Adjust the backdrop stacking level along with the modal when setting a lower `ZIndex`.
</issue_to_address>
### Comment 5
<location path="src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs" line_range="427-430" />
<code_context>
<div @attributes="@AdditionalAttributes" class="@ClassString">
<div class="picker-panel-body">
@if (ShowSidebar)
{
+
<div class="picker-panel-sidebar">
</code_context>
<issue_to_address>
**Failed opening prevents reopening**
When modal readiness or the `showModal` JavaScript interop call fails, `TriggerShowModalCallback` sets `_isModalOpen` before awaiting modal readiness and the JavaScript `showModal` call. If either operation fails, the flag remains true, so later requests skip opening and the picker stays unusable until the component is recreated.
Reset `_isModalOpen` when opening fails, then propagate or log the error so a later request can retry.
</issue_to_address>
### Comment 6
<location path="src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs" line_range="438-439" />
<code_context>
+
+ private Task OnModalClosedAsync()
+ {
+ _isModalOpen = false;
+ return TriggerHideCallback();
+ }
+
</code_context>
<issue_to_address>
**Immediate reopen is ignored**
When the user reopens the picker immediately after the modal’s hidden event, before the asynchronous .NET close callback runs, `TriggerShowModalCallback` sees `_isModalOpen` still true and returns without calling `showModal`, so the picker stays closed after the close callback eventually clears the flag.
Clear `_isModalOpen` synchronously when the modal begins closing, or otherwise serialize close completion with new open requests.
</issue_to_address>
### Comment 7
<location path="test/UnitTest/Components/DateTimePickerTest.cs" line_range="343" />
<code_context>
+ pb.Add(a => a.AutoClose, autoClose);
+ });
+ await cut.InvokeAsync(() => cut.FindComponent<Responsive>().Instance.OnResize(BreakPoint.Small));
+ await cut.InvokeAsync(async () => await cut.Instance.TriggerShowModalCallback());
+
+ await cut.FindAll(".date-table .cell").First(el => el.TextContent == "16").ClickAsync();
</code_context>
<issue_to_address>
**Mobile opening path is bypassed**
When the mobile click handler or panel relocation is broken, `Mobile_Confirm_Ok` calls `TriggerShowModalCallback` directly, then clicks the date cell still rendered under the picker at line 345. It never exercises the mobile click handler or requires the panel to be hosted in the modal, so broken mobile opening or panel relocation still passes.
Exercise the trigger click through the mobile interaction path and assert the picker panel is hosted in the modal before interacting with it.
</issue_to_address>Linked issues
#8662 feat(DateTimePicker): 让组件适应移动端需求 · 1 of 1 met
| # | Requirement | Status | Notes |
|---|---|---|---|
| 1 | 让 DateTimePicker 适应移动端需求 | ✅ Met | 在 Medium 以下断点使用弹窗展示，并为移动端侧边栏提供可展开布局。 |
Sourcery assessment
Approval pending. 7 findings to address first.
Blocking findings: src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js:22, src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js:23, src/BootstrapBlazor/Components/Modal/Modal.razor:6, src/BootstrapBlazor/Components/Modal/Modal.razor.cs:31, src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs:430, and 2 more
Comment on lines
+18
to
+22
| const mobile = el.getAttribute('data-bb-mobile'); | ||
| if (mobile !== 'false') { | ||
| e.preventDefault(); | ||
| e.stopImmediatePropagation(); | ||
| if (mobile === 'true') { |
Contributor
There was a problem hiding this comment.
🟡 Medium · First picker click does nothing
When the picker is clicked before the initial responsive breakpoint callback sets data-bb-mobile, the click handler prevents the default action and stops propagation while the value is not false, but requests a modal only when it is true; the first click therefore opens neither the modal nor the popover.
Handle the unset mobile state so an early click opens the appropriate picker presentation without suppressing the desktop popover.
Also at src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs:386.
Prompt for AI agents
In `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js` at lines 18-22:
**First picker click does nothing**
When the picker is clicked before the initial responsive breakpoint callback sets `data-bb-mobile`, the click handler prevents the default action and stops propagation while the value is not `false`, but requests a modal only when it is `true`; the first click therefore opens neither the modal nor the popover.
Handle the unset mobile state so an early click opens the appropriate picker presentation without suppressing the desktop popover.
Also at `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs:386`.|
|
||
| <BootstrapBlazorRootContent> | ||
| <div @attributes="@AdditionalAttributes" class="@ClassString" tabindex="-1" role="dialog" id="@Id" | ||
| <div @attributes="@AdditionalAttributes" class="@ClassString" style="@StyleString" tabindex="-1" role="dialog" id="@Id" |
Contributor
There was a problem hiding this comment.
🟡 Medium · Caller modal styles disappear
When a caller supplies a style attribute through AdditionalAttributes, the explicit style attribute uses StyleString, which contains only the generated z-index declaration and does not include the caller's style; the caller's inline styles are missing from the rendered modal.
Merge the caller-provided style from AdditionalAttributes with the generated z-index style.
Also at src/BootstrapBlazor/Components/Modal/Modal.razor.cs:30-32, src/BootstrapBlazor/Components/Modal/Modal.razor:7.
Prompt for AI agents
In `src/BootstrapBlazor/Components/Modal/Modal.razor` at line 6:
**Caller modal styles disappear**
When a caller supplies a `style` attribute through `AdditionalAttributes`, the explicit `style` attribute uses `StyleString`, which contains only the generated z-index declaration and does not include the caller's style; the caller's inline styles are missing from the rendered modal.
Merge the caller-provided style from `AdditionalAttributes` with the generated z-index style.
Also at `src/BootstrapBlazor/Components/Modal/Modal.razor.cs:30-32`, `src/BootstrapBlazor/Components/Modal/Modal.razor:7`.| .Build(); | ||
|
|
||
| private string? StyleString => CssBuilder.Default() | ||
| .AddClass($"--bs-modal-zindex:{ZIndex};", ZIndex.HasValue) |
Contributor
There was a problem hiding this comment.
🟡 Medium · Backdrop covers low-z-index modal
When ZIndex is set below Bootstrap's default backdrop z-index of 1050, ZIndex changes only the modal's stacking level, leaving the backdrop at its default level, so the backdrop covers the dialog and makes it inaccessible.
Adjust the backdrop stacking level along with the modal when setting a lower ZIndex.
Prompt for AI agents
In `src/BootstrapBlazor/Components/Modal/Modal.razor.cs` at line 31:
**Backdrop covers low-z-index modal**
When `ZIndex` is set below Bootstrap's default backdrop z-index of 1050, `ZIndex` changes only the modal's stacking level, leaving the backdrop at its default level, so the backdrop covers the dialog and makes it inaccessible.
Adjust the backdrop stacking level along with the modal when setting a lower `ZIndex`.
Comment on lines
+427
to
+430
| { | ||
| _isModalOpen = true; | ||
| var modal = await _modalReady.Task; | ||
| await InvokeVoidAsync("showModal", Id, modal.Id); |
Contributor
There was a problem hiding this comment.
🟡 Medium · Failed opening prevents reopening
When modal readiness or the showModal JavaScript interop call fails, TriggerShowModalCallback sets _isModalOpen before awaiting modal readiness and the JavaScript showModal call. If either operation fails, the flag remains true, so later requests skip opening and the picker stays unusable until the component is recreated.
Reset _isModalOpen when opening fails, then propagate or log the error so a later request can retry.
Prompt for AI agents
In `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs` at lines 427-430:
**Failed opening prevents reopening**
When modal readiness or the `showModal` JavaScript interop call fails, `TriggerShowModalCallback` sets `_isModalOpen` before awaiting modal readiness and the JavaScript `showModal` call. If either operation fails, the flag remains true, so later requests skip opening and the picker stays unusable until the component is recreated.
Reset `_isModalOpen` when opening fails, then propagate or log the error so a later request can retry.
Comment on lines
+438
to
+439
| _isModalOpen = false; | ||
| return TriggerHideCallback(); |
Contributor
There was a problem hiding this comment.
🟡 Medium · Immediate reopen is ignored
When the user reopens the picker immediately after the modal’s hidden event, before the asynchronous .NET close callback runs, TriggerShowModalCallback sees _isModalOpen still true and returns without calling showModal, so the picker stays closed after the close callback eventually clears the flag.
Clear _isModalOpen synchronously when the modal begins closing, or otherwise serialize close completion with new open requests.
Prompt for AI agents
In `src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.cs` at lines 438-439:
**Immediate reopen is ignored**
When the user reopens the picker immediately after the modal’s hidden event, before the asynchronous .NET close callback runs, `TriggerShowModalCallback` sees `_isModalOpen` still true and returns without calling `showModal`, so the picker stays closed after the close callback eventually clears the flag.
Clear `_isModalOpen` synchronously when the modal begins closing, or otherwise serialize close completion with new open requests.| pb.Add(a => a.AutoClose, autoClose); | ||
| }); | ||
| await cut.InvokeAsync(() => cut.FindComponent<Responsive>().Instance.OnResize(BreakPoint.Small)); | ||
| await cut.InvokeAsync(async () => await cut.Instance.TriggerShowModalCallback()); |
Contributor
There was a problem hiding this comment.
🟡 Medium · Mobile opening path is bypassed
When the mobile click handler or panel relocation is broken, Mobile_Confirm_Ok calls TriggerShowModalCallback directly, then clicks the date cell still rendered under the picker at line 345. It never exercises the mobile click handler or requires the panel to be hosted in the modal, so broken mobile opening or panel relocation still passes.
Exercise the trigger click through the mobile interaction path and assert the picker panel is hosted in the modal before interacting with it.
Prompt for AI agents
In `test/UnitTest/Components/DateTimePickerTest.cs` at line 343:
**Mobile opening path is bypassed**
When the mobile click handler or panel relocation is broken, `Mobile_Confirm_Ok` calls `TriggerShowModalCallback` directly, then clicks the date cell still rendered under the picker at line 345. It never exercises the mobile click handler or requires the panel to be hosted in the modal, so broken mobile opening or panel relocation still passes.
Exercise the trigger click through the mobile interaction path and assert the picker panel is hosted in the modal before interacting with it.feat:适配移动端弹窗模式 doc:修改相应文档描述 test:补齐单元测试
Tony-ST0754
changed the title
test:补充单元测试
feat:补充在未选择开始时间时，确定按钮不可用 test:补充单无测试
true 时创建 dialog 否则 popup
使用 dialog 包裹 PickerBody 组件（逻辑不变）
* doc: 完善说明 * doc: 更新样式 # Conflicts: # src/BootstrapBlazor.Server/Components/Samples/DateTimePickers.razor
# Conflicts: # src/BootstrapBlazor/Components/DateTimePicker/DatePickerBody.razor # src/BootstrapBlazor/Components/DateTimePicker/DatePickerBody.razor.cs # src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor # src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.js # src/BootstrapBlazor/Components/DateTimePicker/DateTimePicker.razor.scss # src/BootstrapBlazor/Components/DateTimeRange/DateTimeRange.razor # src/BootstrapBlazor/Components/DateTimeRange/DateTimeRange.razor.cs # src/BootstrapBlazor/Components/DateTimeRange/DateTimeRange.razor.scss # src/BootstrapBlazor/Locales/en.json # src/BootstrapBlazor/Locales/zh.json
ArgoZhang
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #8662
解决方案 / Solution
Zindex参数，用以控制在移动端的弹窗层级DateTimeRange时间范围组件在保持现有模式不变，在移动端也采取弹窗模式，且为单面板模式DateTimeRange在弹窗标题栏上新增重选按钮功能，以处理在移动端单面板模式下，用户重新选择开始时间，避免必须完成选择结束时间后，再重新选择始起时间Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable responsive modal-based date and date-range selection on mobile devices while retaining the existing desktop experience.
New Features:
Enhancements:
Tests:
Chores: