-
Notifications
You must be signed in to change notification settings - Fork 7
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
feature(features:main:theme): add theme manager for switch between dark
and light
themes
#33
Conversation
…ark` and `light` themes
fontSize: 36, | ||
height: 52 / 36, | ||
leadingDistribution: TextLeadingDistribution.even, | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a method to reduce code duplication. For ex
static TextStyle _createCommonTextStyle({
required String fontFamily,
required Color color,
required FontStyle fontStyle,
required FontWeight fontWeight,
required double fontSize,
required double height,
required TextLeadingDistribution leadingDistribution,
}) {
return TextStyle(
fontFamily: fontFamily,
color: color,
fontStyle: fontStyle,
fontWeight: fontWeight,
fontSize: fontSize,
height: height,
leadingDistribution: leadingDistribution,
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phoenixit99 , Understood. This is a valid point, and I’ll address it in a separate issue at a more appropriate time, aligned with issue #34
color: SurfacePallet.light.surface3, | ||
fontStyle: FontStyle.normal, | ||
fontWeight: FontWeight.w400, | ||
fontSize: 32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider defining the font size as a constant
static const fontSizes = FontSizes(
displaySmall: 16,
headlineLarge: 36,
// ... other sizes
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all the review items are related to text styles, we already have a dedicated issue addressing this.
We can finishing this merge request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.got it. We can merge request the PR
No description provided.