Skip to content

Conversation

@danielchalmers
Copy link
Member

@danielchalmers danielchalmers commented Jul 17, 2024

As this is technically a breaking change (more info in the linked PRs) this will be a part of v8.x.x, not v7.x.x.

@danielchalmers danielchalmers added enhancement Request for adding a new feature or enhancing existing functionality (not fixing a defect) breaking change This change will require consumer code updates and removed PR: needs review labels Jul 17, 2024
@danielchalmers
Copy link
Member Author

@ScarletKuro I think you had another idea for this but I don't remember what it was.

@codecov
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.85%. Comparing base (28bc599) to head (0563922).
Report is 526 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9434      +/-   ##
==========================================
+ Coverage   89.82%   90.85%   +1.02%     
==========================================
  Files         412      415       +3     
  Lines       11878    12969    +1091     
  Branches     2364     2510     +146     
==========================================
+ Hits        10670    11783    +1113     
+ Misses        681      619      -62     
- Partials      527      567      +40     

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

@ScarletKuro
Copy link
Member

@ScarletKuro I think you had another idea for this but I don't remember what it was.

just make BaseTypography abstract, then it will give a compiler error when new unspecified type is used.

@danielchalmers
Copy link
Member Author

@jperson2000 What do you think about this remark?

image

@ScarletKuro
Copy link
Member

ScarletKuro commented Sep 18, 2024

Can we also rename this pls:

public class Default : BaseTypography

from Default to DefaultTypography.

The problem I'm facing is that I want to use JSON source generator in ThemeManager as the deepclone fails when trimming / aot used. But I can't use sourcegen as the generator fails on this part because the class and property have the same name :/ the sourgen nests the classes inside.

@ScarletKuro
Copy link
Member

ScarletKuro commented Oct 7, 2024

Wondering if we should just add ICloneable to whole MudTheme considering that it's very logical to have it deep clonable and it would be faster than STJ source-generator. The only problem is it's easy to forget to add new property to the Clone method.

@ScarletKuro ScarletKuro merged commit 0bb82fe into MudBlazor:dev Oct 10, 2024
4 checks passed
@ScarletKuro
Copy link
Member

Added to v8.0.0 Migration Guide #9953

@danielchalmers
Copy link
Member Author

@ScarletKuro If Default became DefaultTypography, should the others gain this suffix? H3 -> H3Typography

@ScarletKuro
Copy link
Member

ScarletKuro commented Oct 10, 2024

@ScarletKuro If Default became DefaultTypography, should the others gain this suffix? H3 -> H3Typography

That makes sense to me. It feels odd to have common names like Default, Button, Input etc under the root (MudBlazor namespace), as there’s a good chance of conflicts.
However, I’ll leave that decision up to @henon. I wouldn’t want to do too many renamings here and there.

I only renamed Default because it broke the STJ source generator for some reason(not for the first time I'm seeing this problem). I'm not sure why. My initial thought was that the source generator creates nested code like this:

public class Generated
{
    public Default Default { get; set; } = new();

    public class Default {}
}

This would throw an exception in C#, but it doesn’t for other classes like H1, H2, etc. So only the necessary part was renamed and I couldn’t find a better name than DefaultTypography, since Typography was already taken.

@ScarletKuro
Copy link
Member

@ebendorland it's not part of v7.9.0, it's for next major version.

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 enhancement Request for adding a new feature or enhancing existing functionality (not fixing a defect)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change theme class Typography members to type BaseTypography

2 participants