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

MudPopover: Add DropShadow property #8938

Merged
merged 4 commits into from
May 11, 2024

Conversation

Yomodo
Copy link
Contributor

@Yomodo Yomodo commented May 10, 2024

Description

Added bool property DropShadow to MudPopover (in preparation of a PR for #6995)
Added int property Elevation to MudGlobal.Popover class with a default value of: 8.

DropShadow true (default)

image

DropShadow false (Elevation 0)

image

How Has This Been Tested?

Unit: Added two tests
Visually

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 May 10, 2024
Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.49%. Comparing base (28bc599) to head (825b21e).
Report is 177 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8938      +/-   ##
==========================================
+ Coverage   89.82%   90.49%   +0.66%     
==========================================
  Files         412      419       +7     
  Lines       11878    12191     +313     
  Branches     2364     2380      +16     
==========================================
+ Hits        10670    11032     +362     
+ Misses        681      627      -54     
- Partials      527      532       +5     

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

Copy link
Contributor

@danielchalmers danielchalmers left a comment

Choose a reason for hiding this comment

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

Not a fan of having two properties to control the same thing and I don't know any other components that do this, but the others are fine with it and I get why it's useful

src/MudBlazor/Services/MudGlobal.cs Outdated Show resolved Hide resolved
src/MudBlazor.UnitTests/Components/PopoverTests.cs Outdated Show resolved Hide resolved
src/MudBlazor/Components/Popover/MudPopover.razor.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@danielchalmers danielchalmers left a comment

Choose a reason for hiding this comment

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

Thank you ❤️

/// <summary>
/// The higher the number, the heavier the drop-shadow.
/// </summary>
[Parameter]
[Category(CategoryTypes.Popover.Appearance)]
public int Elevation { set; get; } = 8;
public int Elevation { set; get; } = MudGlobal.PopoverDefaults.Elevation;
Copy link
Collaborator

@henon henon May 10, 2024

Choose a reason for hiding this comment

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

We could discuss removing this parameter entirely. You know, just use MudGlobal.PopoverDefaults.Elevation directly and disallow setting individual elevation for each popover. I just don't see why you would need different elevation levels for different popups.

Pretty sure @danielchalmers would agree to this. What about you @ScarletKuro ? @Mr-Technician ? @JonBunator ? @Garderoben ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that's better. Menu has a PopoverClass so users can add it themselves (mud-elevation-3) along with other specialized customization. For the average person it removes complexity from the library (and maintenance for contributors).

Copy link
Member

Choose a reason for hiding this comment

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

Disallow setting individual elevation for each popover. I just don't see why you would need different elevation levels for different popups.

Work experience shows that businesses sometimes have strange ideas about UI/UX, so I prefer to have control over everything. That's why I'd ask the community what they think. I don't have a strong opinion on this, as my MudBlazor usage is limited to my personal projects and not related to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was running into an issue with the Drawer with a simple fix but it became a nightmare because we had to deal with edge cases with the scrollbars (for probably only a handful of users) so I gave up. At some point excessive customization and supporting every combination becomes restricting so it may be good to put your foot down and make people do it "the right way". This isn't specifically about this but more of a general point I want to make.

Copy link
Member

Choose a reason for hiding this comment

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

I could see possible use cases for setting different elevations across multiple Popovers. I'm not sure I like pulling global configuration with no way to override it (outside of adding a class manually).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it is hard to argue with that.

@henon henon merged commit d3bfeaa into MudBlazor:dev May 11, 2024
4 checks passed
@Yomodo Yomodo deleted the MudPopover_Add_DropShadow_Property branch May 11, 2024 14:04
@Yomodo Yomodo mentioned this pull request May 11, 2024
6 tasks
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

5 participants