feat(very_good_app_ui): add responsive typography system#520
Open
juanRodriguez17 wants to merge 6 commits intomainfrom
Open
feat(very_good_app_ui): add responsive typography system#520juanRodriguez17 wants to merge 6 commits intomainfrom
juanRodriguez17 wants to merge 6 commits intomainfrom
Conversation
marcossevilla
requested changes
May 4, 2026
| static const TextStyle displayLargeDesktop = TextStyle( | ||
| fontSize: 48, | ||
| fontWeight: FontWeight.w700, | ||
| height: 56 / 48, |
Member
There was a problem hiding this comment.
can't we add the value directly instead of performing an operation? same for the ones below
|
|
||
| /// Returns the appropriate TextTheme based on screen width. | ||
| /// Uses 600px as the breakpoint (same as ResponsiveScaffold). | ||
| static TextTheme getResponsiveTextTheme(BuildContext context) { |
Member
There was a problem hiding this comment.
can we avoid passing BuildContext here? since it's a huge object that changes rapidly, we want to rely directly on specific properties of it like Size
Comment on lines
+305
to
+310
| /// ============ THEME EXTENSION ============ | ||
| @override | ||
| AppTextStyles copyWith() => this; | ||
|
|
||
| @override | ||
| AppTextStyles lerp(AppTextStyles? other, double t) => this; |
Member
There was a problem hiding this comment.
are these necessary since they just return the same instance? let's remove them if we can
| group('desktop text styles', () { | ||
| test('displayLargeDesktop has correct values', () { | ||
| const style = AppTextStyles.displayLargeDesktop; | ||
| expect(style.fontSize, 48); |
Member
There was a problem hiding this comment.
Suggested change
| expect(style.fontSize, 48); | |
| expect(style.fontSize, equals(48)); |
prefer using matches instead of raw values
Comment on lines
+19
to
+35
|
|
||
| # Uncomment and update the font family name and assets once font files are | ||
| # added under assets/fonts/. Also update AppTextStyles.fontFamily accordingly. | ||
| # flutter: | ||
| # fonts: | ||
| # - family: CustomFont | ||
| # fonts: | ||
| # - asset: assets/fonts/CustomFont-Light.ttf | ||
| # weight: 300 | ||
| # - asset: assets/fonts/CustomFont-Regular.ttf | ||
| # weight: 400 | ||
| # - asset: assets/fonts/CustomFont-Medium.ttf | ||
| # weight: 500 | ||
| # - asset: assets/fonts/CustomFont-SemiBold.ttf | ||
| # weight: 600 | ||
| # - asset: assets/fonts/CustomFont-Bold.ttf | ||
| # weight: 700 |
Member
There was a problem hiding this comment.
let's remove these comments, we can add it to the README if it's important documentation
github-project-automation
Bot
moved this from Needs Triage
to In Review
in VGV Open Source 🦄 🧙🌟
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
AppTextStyleswith desktop and mobile staticTextStyleconstants covering the Material 3 type scaledesktopTextTheme,mobileTextThemegetters and agetResponsiveTextTheme(BuildContext)helper (with a defined breakpoint)AppTextStylesas aThemeExtensionin both light and darkAppThemeIssue #521
Type of Change