Skip to content

Conversation

@danielchalmers
Copy link
Member

@danielchalmers danielchalmers commented Dec 23, 2024

Description

See #10378 (comment)

  • Removed lesser used defaults or one that should be set on a per-component basis like Stack.Reverse.
  • Removed ButtonDefaults.Size because Size is a scaling property that should be handled elsewhere
  • Removed comments to avoid another vector for maintenance
    • No one should be learning these properties through MudGlobal itself
    • EXCEPT in cases where the defaults span multiple properties
    • they are now properly referenced from the original property instead
  • MudGlobal.Rounded added to control rounding properties on all components
    • Consider this a working proof of concept of the new direction we're moving in
    • Note that it's a single property and not spread among several *Defaults classes

We need to focus on fundamental components and layouts instead of high level ones like DataGrid.

How Has This Been Tested?

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • 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)

Checklist

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

@github-actions github-actions bot added the breaking change This change will require consumer code updates label Dec 23, 2024
@danielchalmers danielchalmers changed the title Strip mud global MudGlobal: Remove low-value properties and revise all comments Dec 23, 2024
@danielchalmers danielchalmers changed the title MudGlobal: Remove low-value properties and revise all comments MudGlobal: Remove properties with low impact Dec 23, 2024
@danielchalmers
Copy link
Member Author

@jperson2000 This may impact your API feature - apologies for the reversal! Also I changed comments if you could take a look and would that be the reason ApiMemberTable_RenderGlobals_WhenExisting no longer passes?

@danielchalmers danielchalmers added the API change Modifies the public API label Dec 23, 2024
/// Defaults to <c>false</c>.
/// </remarks>
[Parameter]
public bool Dense { get; set; } = MudGlobal.DataGridDefaults.Dense;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a global dense might make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't be complete without also affecting the spacing and margin properties, that would be my concern

Copy link
Member Author

@danielchalmers danielchalmers Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs a ground-up overhaul, see: #8946

@henon henon removed the request for review from jperson2000 December 23, 2024 18:23
@henon
Copy link
Collaborator

henon commented Dec 23, 2024

Makes sense.

@danielchalmers danielchalmers marked this pull request as ready for review December 23, 2024 18:49
@danielchalmers danielchalmers changed the title MudGlobal: Remove properties with low impact MudGlobal: Remove low impact properties Dec 23, 2024
@Anu6is
Copy link
Contributor

Anu6is commented Dec 23, 2024

Why remove global button size?
Also does DataGrid need global spacing?

@jperson2000
Copy link
Contributor

@jperson2000 This may impact your API feature - apologies for the reversal! Also I changed comments if you could take a look and would that be the reason ApiMemberTable_RenderGlobals_WhenExisting no longer passes?

Hi Daniel! I'm out for the holidays but can take a look at this on the 26th. I think this test just has to be changed to point to any existing MudGlobal property, so it can test that ApiMemberTable works. Any property is fine.

@danielchalmers
Copy link
Member Author

Friendly reminder @jperson2000 :)

@michaelhofer-slg
Copy link

I just stumbled over this PR and wonder why some MudGlobals, on which we rely on, are removed while there is an open PR to extend this (as mentioned in the PR description: #10378).

May I ask for the reason why trimming down the amount of the MudGlobals is preferred? Personally, I am in favour of the changes proposed in #10378, which would provide greater flexibility if I am not mistaken.

@henon
Copy link
Collaborator

henon commented Jan 9, 2025

@michaelhofer-slg There are certainly some globals which are unnecessary because it is easy enough to set their value on the component directly, i.e. some of the DataGridDefaults. MudDataGrid usage is surely not comparable to MudButton which is used much more frequently and thus the impact of global default values is much higher. We want to get rid of the low-impact defaults to keep the globals manageable as a whole.

That being said, we'll listen to the community. Please let us know which properties you have to set over and over (and not just a couple of times per application!) which should not be removed.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2025

@henon henon merged commit 341f36c into MudBlazor:dev Jan 9, 2025
4 checks passed
@henon
Copy link
Collaborator

henon commented Jan 9, 2025

That being said, we'll listen to the community. Please let us know which properties you have to set over and over (and not just a couple of times per application!) which should not be removed.

Those will be done in follow up PRs.

@ScarletKuro
Copy link
Member

Removing button size was a mistake, I didn't pay attention.
I can imagine this property to be popular.

@Anu6is
Copy link
Contributor

Anu6is commented Jan 19, 2025

I did ask about https://github.com/MudBlazor/MudBlazor/pull/10516#issuecomment-2560358907. Think my comment got buried though. I didn't revisit this PR after that, should have followed up.

@TheBigNeo
Copy link

@henon
Copy link
Collaborator

henon commented Jan 19, 2025

If I understand correctly @danielchalmers wanted the button size to be somehow automatic similar to what he implemneted with menu icon size but I think we still need individually controlled sizing even if there is some automatism regarding the button's size and in that regard having it in the globals makes sense (it would override automatic button size).

@ScarletKuro
Copy link
Member

automatic similar to what he implemneted with menu icon size

I think it was cancelled in the end #10627 (comment) as it's now always medium

@danielchalmers
Copy link
Member Author

Why Button.Size and not Switch.Size, Slider.Size, Avatar.Size etc? Would it apply to IconButton, ButtonGroup, ToggleGroup?

@danielchalmers
Copy link
Member Author

If I understand correctly @danielchalmers wanted the button size to be somehow automatic similar to what he implemneted with menu icon size

I don't remember automatic button sizing, might have been a misunderstanding.

it's now always medium

Correct, I realized Material Design went this way

@henon
Copy link
Collaborator

henon commented Jan 19, 2025

Sorry, I think I mixed up Dense and Size, both things one needs to control individually but also potentially default to a certain value for every instance.

@henon
Copy link
Collaborator

henon commented Jan 19, 2025

Why Button.Size and not Switch.Size, Slider.Size, Avatar.Size etc? Would it apply to IconButton, ButtonGroup, ToggleGroup?

Oh right. Now I see what you mean. Would it be stupid to set Size that defaults it for everything? Maybe ;). Guess it was correct to remove it after all.

@danielchalmers
Copy link
Member Author

Why Button.Size and not Switch.Size, Slider.Size, Avatar.Size etc? Would it apply to IconButton, ButtonGroup, ToggleGroup?

Oh right. Now I see what you mean. Would it be stupid to set Size that defaults it for everything? Maybe ;). Guess it was correct to remove it after all.

Something that applies a density to the whole application (regardless of component) would be better but takes a lot of work to implement #8946

@ScarletKuro
Copy link
Member

Why Button.Size and not Switch.Size, Slider.Size, Avatar.Size etc? Would it apply to IconButton, ButtonGroup, ToggleGroup?

But all the components you listed are niche, while button is not. Button is widely used in other components as well, like the DataGrid and you can't replace many things with own RenderFragment or apply individual settings.

@ssttgg
Copy link

ssttgg commented Jan 19, 2025

I've just been caught out by this PR when upgrading to v8. A lot of the globals that we were relying on to reduce noise in our markup have now vanished and I don't see any mention of an alternative in the migration guide or this PR. I'm now faced with the prospect of going through my codebase and adding in properties that were previously being default.

I do feel like if you wanted MudGlobal to become something else, then you should have introduced that new thing alongside rather than making a breaking change.

I hope your mind is not made up about this feature. In our experience, we found it very useful to be set global defaults for all components and were looking forward to #10378. It helped us lessen the pressure to write custom code to get a site-wide feel locked down.

@Anu6is
Copy link
Contributor

Anu6is commented Jan 19, 2025

@ssttgg what are some of the common globals you used?

@TheBigNeo
Copy link

After discussing with our team, we are open to preparing Pull Request #10378 again to make it suitable for merging. However, we would need assurance that the changes will be utilized before proceeding.

We are happy to adjust the properties and remove any that are not desired. To ensure alignment, we suggest discussing which properties should not be included before moving forward. Please let us know your thoughts!

We need the properties that I added in the PR #10378.

@ssttgg
Copy link

ssttgg commented Jan 19, 2025

@ssttgg what are some of the common globals you used?

All of them realy, but in particular missing the globals removed on MudCard, MudPaper and MudDataGrid (elevation, outlined etc.) as we have a lot of those on the site.

@hexpoint
Copy link

Why Button.Size and not Switch.Size, Slider.Size, Avatar.Size etc? Would it apply to IconButton, ButtonGroup, ToggleGroup?

But all the components you listed are niche, while button is not. Button is widely used in other components as well, like the DataGrid and you can't replace many things with own RenderFragment or apply individual settings.

We have hundreds of buttons and the majority of them are Small while only a few are Medium or Large. It was nice to have removed the noise of Size=Small from hundreds of buttons. Would prefer to see that one global have stayed as is, the rest are lesser impact.

@danielchalmers danielchalmers removed their assignment Jan 21, 2025
@danielchalmers
Copy link
Member Author

Hi, thanks for the feedback! Please create a new issue proposal or discussion with specific additions in mind so we can discuss specifics. Note: We will favor a low number of properties (that cascade to multiple different components) over 1:1 property mapping. That said we are always open to ideas.

Overriding CSS is sometimes the most effective way of providing consistent styles in your app, but it's perfectly understandable if C# is preferred.

@MudBlazor MudBlazor locked as resolved and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

API change Modifies the public API breaking change This change will require consumer code updates

Projects

No open projects
Status: In progress

Development

Successfully merging this pull request may close these issues.

9 participants