Skip to content

MudPopover: Remove MudGlobal.PopoverDefaults and move properties to PopoverOptions - #12286

Merged
danielchalmers merged 7 commits into
devfrom
copilot/remove-popover-defaults-again
Dec 18, 2025
Merged

MudPopover: Remove MudGlobal.PopoverDefaults and move properties to PopoverOptions#12286
danielchalmers merged 7 commits into
devfrom
copilot/remove-popover-defaults-again

Conversation

Copilot AI commented Dec 17, 2025

Copy link
Copy Markdown
Contributor
  • Add ModalOverlay and OverflowBehavior properties to PopoverOptions
  • Remove MudGlobal.PopoverDefaults class from MudGlobal.cs
  • Update components to properly use PopoverOptions via DI:
    • Inject IPopoverService into MudMenu, MudAutocomplete, MudSelect, MudPicker
    • Change Modal and OverflowBehavior parameters to nullable types
    • Add GetModal() and GetOverflowBehavior() resolver methods that fall back to PopoverOptions
    • Update razor files to use resolver methods
    • Add TODO comments for rewriting once .NET 8 support is dropped
  • Update ServiceCollectionExtensions.cs to pass new properties to popover service
  • Update unit tests:
    • Add new properties to AddMudPopoverService_ShouldRegisterServices_WithOptionsAction
    • Add new properties to AddMudServices_ShouldRegisterAllServices_WithOptionsAction
    • Default null values on unset parameters
    • Component-level parameter overrides
    • Global defaults from PopoverOptions are properly picked up
  • Update documentation page for Popover
  • Build and run tests to verify changes
Original prompt

Remove MudGlobal.PopoverDefaults class and move ModalOverlay and OverflowBehavior to PopoverOptions


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: danielchalmers <7112040+danielchalmers@users.noreply.github.com>
@danielchalmers

danielchalmers commented Dec 17, 2025

Copy link
Copy Markdown
Member

@versile2 Does this seem like a sensible change on the surface?

@danielchalmers
danielchalmers marked this pull request as ready for review December 17, 2025 18:42
@mudbot mudbot Bot changed the title [WIP] Remove MudGlobal.PopoverDefaults class and update PopoverOptions Popover: Remove MudGlobal.PopoverDefaults class and update PopoverOptions Dec 17, 2025
@mudbot mudbot Bot added the refactor Reorganizes code with no changes to the API or functionality in the main library or other benefits label Dec 17, 2025
Co-authored-by: danielchalmers <7112040+danielchalmers@users.noreply.github.com>
Copilot AI changed the title Popover: Remove MudGlobal.PopoverDefaults class and update PopoverOptions Remove MudGlobal.PopoverDefaults and move properties to PopoverOptions Dec 17, 2025
Copilot AI requested a review from danielchalmers December 17, 2025 18:50
@danielchalmers danielchalmers added breaking change This change will require consumer code updates (ex: removes/changes an API) and removed refactor Reorganizes code with no changes to the API or functionality in the main library or other benefits labels Dec 17, 2025
@ScarletKuro

Copy link
Copy Markdown
Member

I'm not seeing a test where he would change the service options, on the DI level and that component that would pick it up?
I don't see the logic where it decides whatever it will pick the local component setting or the global one?

@versile2

Copy link
Copy Markdown
Contributor

This does not implement the new PopoverOptions at all, it creates 2 popover options that are never used as far as I can tell.

@danielchalmers

Copy link
Copy Markdown
Member

I'm not seeing a test where he would change the service options, on the DI level and that component that would pick it up? I don't see the logic where it decides whatever it will pick the local component setting or the global one?

This does not implement the new PopoverOptions at all, it creates 2 popover options that are never used as far as I can tell.

Yeah, what is it thinking? Lol. Sorry, I should've checked the implementation instead of just the API surface

@danielchalmers

Copy link
Copy Markdown
Member

@copilot You need to actually hook up ModalOverlay and OverflowBehavior from PopoverOptions in the components. Right now it's just using the default values. Also ensure that those values on both the component and global level are appropriately tested.

…ection

Co-authored-by: danielchalmers <7112040+danielchalmers@users.noreply.github.com>

Copilot AI commented Dec 17, 2025

Copy link
Copy Markdown
Contributor Author

@copilot You need to actually hook up ModalOverlay and OverflowBehavior from PopoverOptions in the components. Right now it's just using the default values. Also ensure that those values on both the c...

Implemented in commit 8772e7d. Components now:

  1. Inject IPopoverService
  2. Use nullable parameters (bool?, OverflowBehavior?)
  3. Resolve defaults via GetModal() and GetOverflowBehavior() methods that fall back to PopoverService.PopoverOptions

Added tests verifying both global defaults from PopoverOptions and component-level overrides work correctly.

@ScarletKuro

Copy link
Copy Markdown
Member

I guess creating

protected OverflowBehavior GetOverflowBehavior() => OverflowBehavior ?? PopoverService.PopoverOptions.OverflowBehavior;

is one of the available options that doesn't violate any Blazor rules. I know that in .NET 9, they finally added constructor injection as an alternative to property injection (which is set only after the component is constructed). You should be able to set the defaults in the constructor, but since we support .NET 8, I’m not sure if that’s available there. If not, it’s probably best to leave a TODO note to rewrite this once .NET 8 support is dropped, as it will be much simpler.

@ScarletKuro

Copy link
Copy Markdown
Member

Seems the flaky SelectClearableTest which versile just fixed. Let's see if that helped.

@ScarletKuro

Copy link
Copy Markdown
Member

@copilot you didn't add the new properties to ServiceCollectionExtensionsTests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates (ex: removes/changes an API)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants