Skip to content

feat(BbDateRangePicker): add two parameters : DisplayButtons and Auto…#244

Open
Yoannnn wants to merge 1 commit into
blazorblueprintui:developfrom
Yoannnn:develop
Open

feat(BbDateRangePicker): add two parameters : DisplayButtons and Auto…#244
Yoannnn wants to merge 1 commit into
blazorblueprintui:developfrom
Yoannnn:develop

Conversation

@Yoannnn
Copy link
Copy Markdown
Contributor

@Yoannnn Yoannnn commented Mar 16, 2026

…Apply

Description

Add parameters to BbDateRangePicker :

  • DisplayButtons : display or not the Reset and Apply buttons
  • AutoApply : auto apply after selection of dates. In some workflow, users forget to click on Apply to validate the range selection

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)

Testing Checklist

  • Blazor Server
  • Blazor WebAssembly
  • Blazor Hybrid (MAUI)
  • Keyboard navigation / accessibility
  • Dark mode

Related Issues

@mathewtaylor
Copy link
Copy Markdown
Contributor

mathewtaylor commented May 2, 2026

Hi @Yoannnn, thanks for the contribution! Both DisplayButtons and AutoApply are genuinely useful additions. Auto-apply in particular is a common UX expectation that I've had asked about before, so happy to see it on the way in.

That said, in its current shape the PR introduces too much noise into the repo for me to take it as-is. The actual feature is maybe 30 to 40 substantive lines, but it's currently buried in ~150 lines of unrelated formatting churn which makes it really hard to review and bloats the diff. Could you please rework it before we go further?

Here's what needs addressing:


1. Heavy formatting/whitespace churn

Looks like the IDE auto-reformatted large sections of BbDateRangePicker.razor and the code-behind that you didn't actually change. For example, <LucideIcon ... /> becoming <LucideIcon .../>, line-split <span> elements, indentation changes inside <thead>/<tbody>, the ShouldRender boolean chain getting realigned, blank lines added between unrelated statements, etc.

Could you revert the formatting on lines you didn't intentionally change? The goal is for the diff to show only the new feature, not the whole file rewritten.


2. API surface snapshot test is failing

The ComponentsApiSurfaceMatchesBaseline test fails locally on this branch because the two new public parameters aren't reflected in the snapshot. After your changes settle, run:

./scripts/run-tests.sh --accept

and commit the updated .verified.txt so CI is green.


3. Missing XML doc comments on the new parameters

Other parameters in BbDateRangePicker (Class, MinDate, Disabled, etc.) all carry XML doc summaries. DisplayButtons and AutoApply should match that pattern so they show up properly in IntelliSense and in the API reference docs.


4. Parameter declaration style

The new parameters are declared on a single line:

[Parameter] public bool DisplayButtons { get; set; } = true;
[Parameter] public bool AutoApply { get; set; } = false;

Every other parameter in this class uses the multi-line form with the attribute on its own line. Please match the existing style.


5. DisplayButtons=false + AutoApply=false leaves no way to commit

If a consumer hides the buttons but doesn't enable auto-apply, they have no path to commit a selection, clicks just sit in the picker. Worth either guarding against the combination (e.g. ignore DisplayButtons=false when AutoApply=false, with a debug warning) or at least documenting the dependency in the XML doc on DisplayButtons.


6. Unnecessary lambda on the Apply button

@onclick="@(async () => await Apply(true))"

Since Apply returns a Task and autoClose defaults to true, this can stay as the original method group:

@onclick="Apply"

Same applies to the other onclick handlers that gained lambdas, please revert those that were working as method groups before.


7. Demo heading reads a bit awkwardly

"Buttons reset and apply hidden and auto apply"

Something like "Auto apply with buttons hidden" would read more naturally and match the wording style of the other section headings on the page.


Once that's tidied up the change should be much easier to review and ready to merge. Looking forward to it!

Thanks, Mathew

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