Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global: Add transition duration and delay options #8651

Merged
merged 3 commits into from
Apr 14, 2024

Conversation

danielchalmers
Copy link
Contributor

@danielchalmers danielchalmers commented Apr 11, 2024

Description

Resolves #8639 by adding global transition properties that effect the tooltip, popover, and picker.
Part of #4896.

Cleanup:

  • Removes unused MenuItemDebounceInterval
  • Switch to file-scoped namespace

Needs vetting to make sure this is the way to go for this and that I did it the correct way.

How Has This Been Tested?

visually

Types 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)

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 enhancement New feature or request PR: needs review labels Apr 11, 2024
@henon
Copy link
Collaborator

henon commented Apr 12, 2024

The change itself LGTM. However, before we add a ton of these defaults we need to decide some basic rules. Do we add like MudGlobals.DefaultVariant that applies to all buttons, fabs, textfields etc. or do we add defaults grouped by component like MudGlobals.MudButtonDefaults.Variant or maybe even both, so that the MudGlobals.MudButtonDefaults.Variant gets its own default value from MudGlobals.DefaultVariant ?

Or MudGlobals.Defaults.Variant instead of MudGlobals.DefaultVariant

@ScarletKuro @mikes-gh @danielchalmers opinions?

@henon henon requested a review from ScarletKuro April 12, 2024 19:09
@ScarletKuro
Copy link
Member

Do we add like MudGlobals.DefaultVariant that applies to all buttons, fabs, textfields etc. or do we add defaults grouped by component like MudGlobals.MudButtonDefaults.Variant

Honestly, I don't know. This properties (variants, texts etc) are more related to the theme topic than the MudGlobals. I don't think it's good idea to add UI related things to the MudGlobals.
Ideally, I would want something close to Flutter.
You set the properties of the ThemeData at the application startup, then there is a Theme Context accessible in all your widgets (aka components) where you can pick up the global defined property, as example:
color: Theme.of(context).colorScheme.primary.
They put there pretty much everything, text styles, colors, borders, sizes, You can even add your own custom properties through ThemeExtension base class and then call it by using Theme.of(context).extension<AppColorsExtension>().YourProperty

@henon
Copy link
Collaborator

henon commented Apr 12, 2024

Theming is theming, defaults are defaults. Mixing these two together makes everthing very complicated.

@danielchalmers
Copy link
Contributor Author

Might be worth adding them directly to MudGlobal.cs through the v7 previews, and then shaking them out and organizing them in a way that makes sense before v7 GA. It's hard to make a decision with purely hypotheticals

@dennisrahmen
Copy link
Contributor

@henon I would think of defaults as more of a devs thing.

DataGrid Column sorting is false per default so I don't have to turn it off alle the time since most devs only use it in specific cases.

While theming could well be up to the end user to decide.

Choosing a theme that has Filled instead of Outlined.
Like the Dark/Light mode works on the docs.

@henon
Copy link
Collaborator

henon commented Apr 13, 2024

I slept over it. The problem I see with @ScarletKuro's suggestion of a default theme is that it implies that all the values in the theme will be dynamically changeable by switching themes at runtime. That's why I insisted on distinguishing between theming and default settings. The beauty of a default is that it is static. Set once at startup, never change. No need for re-rendering components, etc.

If you want to dynamically switch button variants at runtime like @dennisrahmen suggests above, you end up with a lot more complexity in the components than I'd feel comfortable with. I really don't think it would be worth it (feel free to proof me wrong here). We'd have to switch to nullable values for every parameter and come up with theming logic etc now. And how do you re-render all the affected components? Maybe I am wrong here and it wouldn't be all that big of a deal? It would have to be tested out with a proof of concept PR.

The way I see it, customizable default values seem to be something many need. It is mightly inconvenient to have to set the same parameters over and over again when using mudblazor components, I know that from experience. Although when talking about this problem the discussion always immediately turns toward theming I haven't experienced a need for theming such values myself (which variant to use is a deliberate decision of the designer not the customer's choice, like @dennisrahmen correctly said) and I haven't seen issues requiring the need for dynamically switching button variants and other default values, it seems purely hypothetical like @danielchalmers said. Again, maybe I am ignorant here, so feel free proof me wrong.

If we distinguish between defaults and themable values we make it clear what can be changed dynamically and what can't. So having the defaults in MudGlobals.Defaults instead of a theme would clearly state their purpose. Furthermore, even if we later decide to take the step of making absolutely everything themable, we can still do so. The defaults in MudGlobals would serve as the default values for every unset value in the default theme or any other dynamically set theme.

Conclusion: Adding customizable defaults is dead simple and we don't have to come up with theming logic and design now. It doesn't prevent theming later as themes would override defaults.

I am of course always open to your ideas, I don't want that we go in the wrong direction just because I said so =P

@henon
Copy link
Collaborator

henon commented Apr 13, 2024

Ok, let's go with defaults that apply equally for all components for now. Defaults for single components inheriting from global defaults such as MudGlobals.ButtonDefaults.Variant = MudGlobals.DefaultVariant; and MudGlobals.InputDefaults.Variant = MudGlobals.DefaultVariant; can still be added later without breaking changes.

@ScarletKuro
Copy link
Member

ScarletKuro commented Apr 13, 2024

I understand what @henon is trying to say, but "defaults are default" is more complicated than just that.
I might have a really hard time explaining it, but I will try.
There are two concepts: behavior and appearance.
Behavior: That's delay, duration, maximum file count, error handling, buffer size etc etc etc (I hope you get the idea) that's something that should live in MudGlobal and it's probably not something you'd change during the app lifetime. Or if you do, it's not something that you need to apply immediately on a component. (btw, I don't know why we have duration and delay as appearance category, it's behavior)
Appearance: everything UI related, that must dynamically change immediately as I change it.
Since @henon brought up Variant here - Variant is not a behavior, it's appearance. Those, I MUST be able to change it at runtime. It's an absolutely a theme topic, and it cannot be a static default, and it shouldn't force refresh my page to apply these new "defaults". I know I will be again talking about the "future" that is not there and teams always tell me that we should bother about it when it happens, but yet, let me just explain. What if in future MudBlazor will support multiple themes and then the Variant's default can depend on the theme. What if we will allow users to extend MudBlazor with their own theme and when they have X theme they want Variant.Filled, and on Y they want Variant.Outlined and X and Y change at runtime.
But the main point isn't future, but that I don't want to mix behavior and appearance in single MudGlobal, we should create something with a different naming that is somehow connected to UI.

@henon
Copy link
Collaborator

henon commented Apr 13, 2024

Conclusion: Adding customizable defaults is dead simple and we don't have to come up with theming logic and design now. It doesn't prevent theming later as themes would override defaults.

@ScarletKuro This is a quote from my essay above, just to be clear, I never said having customizable default appearance doesn't allow us to make those things themable later.

@henon
Copy link
Collaborator

henon commented Apr 13, 2024

But the main point isn't future, but that I don't want to mix behavior and appearance in single MudGlobal, we should create something with a different naming that is somehow connected to UI.

Sure, defaults for the appearance could live in another static class if we really need that distinction, which I am not sure. They are still static default values even if they are default values for the appearance. Theming can override them of course.

@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 13, 2024

@henon @ScarletKuro Any others I should add in this PR so we can see what it looks like?

I would say keep it relatively simple for now and save Variant etc for later

Copy link

codecov bot commented Apr 14, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.09%. Comparing base (28bc599) to head (0cb75e2).
Report is 42 commits behind head on dev.

Files Patch % Lines
src/MudBlazor/Services/MudGlobal.cs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8651      +/-   ##
==========================================
+ Coverage   89.82%   90.09%   +0.26%     
==========================================
  Files         412      418       +6     
  Lines       11878    12017     +139     
  Branches     2364     2366       +2     
==========================================
+ Hits        10670    10827     +157     
+ Misses        681      658      -23     
- Partials      527      532       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henon
Copy link
Collaborator

henon commented Apr 14, 2024

@danielchalmers this PR is definitely OK so far. The discussion whether or not we manage defaults for appearance somewhere else can be continued in the team channel.

@henon henon merged commit c807a97 into MudBlazor:dev Apr 14, 2024
4 checks passed
@danielchalmers danielchalmers deleted the global-defaults-tooltip branch April 14, 2024 16:20
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global base tooltip delay
4 participants