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

Globals: Add DialogDefaults.DefaultFocus, Consolidate others to their own static classes #8831

Merged

Conversation

danielchalmers
Copy link
Contributor

Description

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

codecov bot commented Apr 28, 2024

Codecov Report

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

Project coverage is 90.04%. Comparing base (28bc599) to head (b205a85).
Report is 127 commits behind head on dev.

Files Patch % Lines
src/MudBlazor/Services/MudGlobal.cs 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8831      +/-   ##
==========================================
+ Coverage   89.82%   90.04%   +0.21%     
==========================================
  Files         412      421       +9     
  Lines       11878    12292     +414     
  Branches     2364     2437      +73     
==========================================
+ Hits        10670    11068     +398     
+ Misses        681      672       -9     
- Partials      527      552      +25     

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

@henon
Copy link
Collaborator

henon commented Apr 28, 2024

In principle yes, it makes sense to have global defaults for every parameter, even those of MudDialog. I first thought, this would be better suited for DialogOptions but then, it is a parameter of a component and thus the default value would be expected to be customizable like all the other component parameters.

@henon
Copy link
Collaborator

henon commented Apr 28, 2024

When we add defaults for all components, there will be hundreds of definitions in MudGlobal. It might become quite unmanageable. You sure we shouldn't make static subclasses of MudGlobal for every component?

i.e.
MudGlobal.DialogDefaults.DefaultFocus = ...
MudGlobal.ButtonDefaults.Variant = ...

@danielchalmers
Copy link
Contributor Author

@henon Do you want to implement your vision for that API separately and I rebase? Or tell me what the classes would look like and I can do it in this PR?

@henon
Copy link
Collaborator

henon commented Apr 29, 2024

public static class MudGlobal {
  public static class DialogDefaults { 
    public DefaultFocus DefaultFocus = DefaultFocus.Element;
  }
  public static class ButtonDefaults { 
    public static Variant Variant = Variant.Text;
  }
}

@danielchalmers danielchalmers changed the title Dialog: Add MudGlobal.DialogDefaultFocus Globals: Add DialogDefaults.DefaultFocus, Consolidate others to their own static classes Apr 29, 2024
@danielchalmers
Copy link
Contributor Author

@henon Done. Take a look now.

@henon
Copy link
Collaborator

henon commented Apr 29, 2024

Here is another idea, add these on top of what you already have:

    public static class OverlayDefaults
    {
        /// <summary>
        /// The default transition delay for <see cref="MudOverlay"/> and <see cref="MudPicker{T}"/>.
        /// </summary>
        public static TimeSpan Delay { get; set; } = TransitionDefaults.Delay;

        /// <summary>
        /// The default transition time for components like <see cref="MudTooltip"/>, <see cref="MudOverlay"/>, <see cref="MudPicker{T}"/>.
        /// </summary>
        public static TimeSpan Duration { get; set; } = TransitionDefaults.Duration;
    }

    public static class PickerDefaults
    {
        /// <summary>
        /// The default transition delay for <see cref="MudOverlay"/> and <see cref="MudPicker{T}"/>.
        /// </summary>
        public static TimeSpan Delay { get; set; } = TransitionDefaults.Delay;

        /// <summary>
        /// The default transition time for components like <see cref="MudTooltip"/>, <see cref="MudOverlay"/>, <see cref="MudPicker{T}"/>.
        /// </summary>
        public static TimeSpan Duration { get; set; } = TransitionDefaults.Duration;
    }


    public static class TooltipDefaults
    {
        /// <summary>
        /// The default transition delay for <see cref="MudOverlay"/> and <see cref="MudPicker{T}"/>.
        /// </summary>
        public static TimeSpan Delay { get; set; } = TransitionDefaults.Delay;

        /// <summary>
        /// The default transition time for components like <see cref="MudTooltip"/>, <see cref="MudOverlay"/>, <see cref="MudPicker{T}"/>.
        /// </summary>
        public static TimeSpan Duration { get; set; } = TransitionDefaults.Duration;
    }

That way, on top of changing the delay and transition duration for all via TransitionDefaults we can also set them differently for overlay, picker and tooltip.

@danielchalmers
Copy link
Contributor Author

@henon Good idea. Added.

@@ -55,21 +55,21 @@ public partial class MudTooltip : MudComponentBase
/// Sets the length of time that the opening transition takes to complete.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ScarletKuro What do you say, shall we replace all usages of double and int for timespans in the library with TimeSpan while we are at it, in v7?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am talking about this in MudTooltip for instance:

/// Sets the length of time that the opening transition takes to complete.
/// </summary>
/// <remarks>
/// Set globally via <see cref="MudGlobal.TransitionDefaults.Duration"/>.
/// </remarks>
[Parameter]
[Category(CategoryTypes.Tooltip.Appearance)]
public double Duration { get; set; } = MudGlobal.TransitionDefaults.Duration.TotalMilliseconds;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

context: #8574

Copy link
Collaborator

@henon henon left a comment

Choose a reason for hiding this comment

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

@ScarletKuro can be merged from my point of view

@henon henon mentioned this pull request May 1, 2024
2 tasks
@henon henon requested a review from ScarletKuro May 1, 2024 16:27
@ScarletKuro ScarletKuro merged commit 21456c0 into MudBlazor:dev May 1, 2024
3 of 4 checks passed
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.

None yet

3 participants