1696: Apply fonts #1728

f1sh1918 · 2024-10-30T15:49:25Z

Short description

Since we enabled a custom font for koblenz which is applies on the theme. we have to strictly style our texts and text buttons with our theme to apply the fonts everywhere.

Proposed changes

style text with textTheme where they don't inherit from standard widget
add missing styles to flutter standard widgets in theme
fix AppBar text color for ios, now clock and symbols are readable on every page (light theme)
added style overrides to standard widgets styles where needed
cleanup theme declarations in files

Side effects

some text sizes may differ slightly

Testing

Smoke test as much as possible in the native app if sth is broken

Resolved issues

Fixes: #1696

michael-markl · 2024-10-30T16:36:15Z

Have you considered adding a DefaultTextStyle widget instead? This one would be added to the "root" widget and then all Text Widgets would use this text style as their default.

f1sh1918 · 2024-10-30T19:05:35Z

DefaultTextStyle

Hm i could set the fontFamily and fontSize (sth similar as you style html body) with this widget but then i also would have to apply different fontSizes and fontWeights for the particular text.
So i just see one advantage if you forget to set the particular style for a text at least this defaultStyle including fontFamily will apply.
I'm not sure about if you use f.e. flutter ListTile widget that has title and subtile then different fontSizes and fontWeights apply.

I see your point and i may add this additionally but i also see the risk that styles will be forgotten. With this approach we have a standard way to style text elements.
In the end from a design system view you should have a typography that defines all text styles that should be used.

michael-markl · 2024-11-03T12:20:46Z

Hm i could set the fontFamily and fontSize (sth similar as you style html body) with this widget but then i also would have to apply different fontSizes and fontWeights for the particular text.
So i just see one advantage if you forget to set the particular style for a text at least this defaultStyle including fontFamily will apply.

I'm not sure about if you use f.e. flutter ListTile widget that has title and subtile then different fontSizes and fontWeights apply.

After diving a little bit into the theming / styling system built into Flutter, these were my findings:

Most widgets (e.g. TextButton, ListView, AppBar, ...) apply default styles (by "rendering" a DefaultTextStyle widget) that is always based on the theme that we provide via themes.dart. For example, ListView(title: Text("bla")) renders "bla" using the text style defined in theme.textTheme.bodyLarge
If you only want to adjust a few styles for a Text, but keep the otherwise inherited defaults, you can just create a new TextStyle object. Properties that are omitted in the constructor will be inherited from the next default text style in the tree hierarchy above.
Thus, when rendering any standard Material widget (such as buttons, ListTiles, AppBars, etc.) we do not need to adjust any styles (as the appropriate styles are already taken from the theme -- for the details read the docs / implementation of TextButton, ListTile, etc). If we want to adjust some text color for a specific widget, we can simply use Text("my text", style: TextStyle(color: theme.colorScheme.whatever)) and do not need to check which text style is appropriate in the context as the other styles are inherited.

If we render more "custom" widgets - such as the Tappable Cards in the Ausweisen Tab when no card is active - we still should supply theme with the appropriate text style. I believe, however, that even then a reasonable default will be set which is also taken from the theme.

I see your point and i may add this additionally but i also see the risk that styles will be forgotten. With this approach we have a standard way to style text elements.

By now, I believe, we should not render this DefaultTextStyle widget at the root of our app. The flutter MaterialApp widget and friends will supply the correct defaults already. We only have to take care of the ThemeData object.

In the end from a design system view you should have a typography that defines all text styles that should be used.

Yes, I agree. But this design system is already in place and specified by the Material design guidelines. The widgets use these design guidelines when defining their defaults.

f1sh1918 · 2024-11-03T12:49:08Z

Alright thx for this great explanation.
I think a big problem here is the flutter inspector and how to properly check if the theme/style has applied, since these inherited styles often will be shown as null at least I recognized this very often. So I can check which styles I can remove but then it's very difficult to check if f.e. fonts applied correctly if you don't have trained eyes though..

michael-markl · 2024-11-03T13:20:12Z

I think a big problem here is the flutter inspector and how to properly check if the theme/style has applied, since these inherited styles often will be shown as null at least I recognized this very often. So I can check which styles I can remove but then it's very difficult to check if f.e. fonts applied correctly if you don't have trained eyes though..

I see... have you tried to use the tips discussed here? https://stackoverflow.com/a/74263526/3245533

f1sh1918 · 2024-11-05T14:40:48Z

I think a big problem here is the flutter inspector and how to properly check if the theme/style has applied, since these inherited styles often will be shown as null at least I recognized this very often. So I can check which styles I can remove but then it's very difficult to check if f.e. fonts applied correctly if you don't have trained eyes though..

I see... have you tried to use the tips discussed here? https://stackoverflow.com/a/74263526/3245533

Yes i always use this hovering mode. I really find this very difficult and some inherited styles are not should.
If i f.e. remove the text style from an ListTile in about it looks like the correct font family is applied.
If i check the inspector its null and even the dependencies don't give any info about what style is applied from the theme.

For tiles with proper styles applied everything looks fine

I see the point in removing unneeded styles but its super difficult to keep all the docs you mentioned in mind and to double check if everything applied correctly. So i'm not sure which way to go tbh.
Maybe this inspector will be improved in the future.
For now i think my solution is consistent and can be verified even it provides some unneeded styles

michael-markl · 2024-11-08T10:47:23Z

I see the point in removing unneeded styles but its super difficult to keep all the docs you mentioned in mind and to double check if everything applied correctly.

I think my argument would be the exact opposite: It's much harder to always remember the correct style (for e.g. ListTile title, ListTile subtitle, etc.). Therefore, it's much easier to be able to rely on the defaults that are set by ListTile. The fact that they are always based on the theme, makes it easy to know that it always uses the correct font family.

But I agree that the experience of the inspector is quite unpleasant when it comes to inspecting text styles. You have to dig through this dependencies set (expand every "bucket") and eventually you can find the DefaultTextStyle dependency which then tells you the text style it applies.
It seems to be a known issue but it has been open for quite some time, unfortunately: flutter/devtools#3084 flutter/devtools#2272

f1sh1918 · 2024-11-12T10:22:42Z

I see the point in removing unneeded styles but its super difficult to keep all the docs you mentioned in mind and to double check if everything applied correctly.

I think my argument would be the exact opposite: It's much harder to always remember the correct style (for e.g. ListTile title, ListTile subtitle, etc.). Therefore, it's much easier to be able to rely on the defaults that are set by ListTile. The fact that they are always based on the theme, makes it easy to know that it always uses the correct font family.

But I agree that the experience of the inspector is quite unpleasant when it comes to inspecting text styles. You have to dig through this dependencies set (expand every "bucket") and eventually you can find the DefaultTextStyle dependency which then tells you the text style it applies. It seems to be a known issue but it has been open for quite some time, unfortunately: flutter/devtools#3084 flutter/devtools#2272

Okay i think then a good solution would be to define the typography classes like

headline 1
headline xx
title x
body x

And apply them on textTheme and on f.e. subtitle and title of the ListTile f.e. and the other themes we adjust. Then it should automatically rely on that
I mean for ListTiles in particular we have different variants and sizes so just to rely on default values will not work so at any point we have to override them but f.e. for Snackbar or Dialogs that might work

…ables, removed unneeded variables

# Conflicts: # frontend/lib/identification/util/activate_card.dart

f1sh1918 · 2024-11-12T13:12:24Z

@michael-markl i think i moved everything that should be defined in the theme to the themes.dart and removed unneeded styles from the other widgets

michael-markl · 2024-11-18T21:38:39Z

One more remark: Everything inside a Material widget (such as everything inside a Scaffold or a Dialog) uses bodyMedium as DefaultTextStyle (unless it's overwritten by another widget further down the tree).
This means, that unless you're doing something very sketchy (which we don't), everything is themed "correctly" according to the theme. In particular, it's often fine to omit the text style even in "custom" widgets, if we intend to use this default.

ztefanie

deleted. sorry got lost in my browser tabs.

deleted

ztefanie

Code looks good. couldn't test as i need to fix protobuf before.

seluianova

Text of the info items is currently white

Same thing for 'Weitere Aktionen'

Title of the verification result:

seluianova · 2024-11-19T15:32:07Z

frontend/lib/widgets/app_bars.dart

@@ -101,10 +104,10 @@ class SearchSliverAppBarState extends State<SearchSliverAppBar> {
        focusNode: focusNode,
        decoration: InputDecoration.collapsed(
          hintText: t.search.searchHint,
-          hintStyle: TextStyle(color: foregroundColor?.withOpacity(0.8)),
+          hintStyle: theme.textTheme.bodyLarge,


I think the opacity was appropriate here, and this text was white before

alright. yes on dark theme it was fine. this styling stuff is so annoying. thx for the find

yes, I agree 🙈
(I haven't checked all the views yet, maybe something else will arise)

🙃 I don't want to annoy with nit-picky comments, so please leave it as is if you want to, but I can't resist to at least mentioning that 😄
I think it was better with transparency. because currently it's like a label, and not like a background hint as before.

I decided explicitly not to use opacity due to bad readability.
I think the label tells you what to do it's not just a pure placeholder.

I think it's more import that it can be read and users know that there is an input than user don't know that they can type in sth.

But maybe @hauf-toni has an opinion on that

up to you.
I think it's standard for such text to be grayed out.

frontend/lib/themes.dart

seluianova · 2024-11-19T16:06:21Z

frontend/lib/store_widgets/detail/contact_info_row.dart

@@ -44,6 +45,7 @@ class ContactInfoRow extends StatelessWidget {
        Expanded(
          child: Text(
            _description,
+            style: theme.textTheme.bodyMedium,


💭 not sure if that's important, but it was grey before (now it's black)

hm i think thats okay

I personally think it looks more appealing when colors are different, but maybe it's too subjective

f1sh1918 · 2024-11-20T11:46:13Z

@seluianova thx for proper testing. I addressed the issues.
I smoke tested on ios light and dark theme.
Could you please check again on android :)

michael-markl · 2024-11-01T21:34:21Z

frontend/lib/search/filter_bar_button.dart

@@ -102,24 +102,21 @@ class _FilterBarButtonState extends State<FilterBarButton> with SingleTickerProv
                  widget.onCategoryPress(widget.asset, isSelected);
                });
              },
-              child: Padding(
-                padding: const EdgeInsets.only(top: 4),


The padding should be readded.

michael-markl · 2024-11-01T21:59:00Z

frontend/lib/search/filter_bar_button.dart

@@ -87,7 +87,7 @@ class _FilterBarButtonState extends State<FilterBarButton> with SingleTickerProv
      builder: (context, child) {
        final color = Color.lerp(theme.colorScheme.background, selectedColor, colorTween.value);
        return ConstrainedBox(
-          constraints: BoxConstraints.tightFor(width: width, height: 70),
+          constraints: BoxConstraints.tightFor(width: width, height: 74),


Was there a specific reason to increase it? Maybe because of the different Koblenz font, we need more space?

Indeed. the font exceeds line height. Maybe we could also decrease lineHeight

michael-markl · 2024-11-01T23:17:58Z

frontend/lib/store_widgets/accepting_store_summary.dart

          ),
          const SizedBox(height: 4),
          Text(
            store.description ?? t.store.noDescriptionAvailable,
            maxLines: 1,
            overflow: TextOverflow.ellipsis,
-            style: Theme.of(context).textTheme.bodyMedium,
+            style: theme.textTheme.bodyLarge?.apply(color: theme.hintColor),


Is it intentional that you changed this to bodyLarge?

seluianova · 2024-11-20T13:54:18Z

...nd/lib/identification/verification_workflow/dialogs/negative_verification_result_dialog.dart

@@ -15,7 +15,7 @@ class NegativeVerificationResultDialog extends StatelessWidget {
      title: t.identification.notVerified,
      icon: Icons.error,
      iconColor: Colors.red,
-      child: Text(reason),
+      child: Text(reason, style: Theme.of(context).textTheme.bodyLarge),


🙃 The content is bigger than the title. Can we apply some style to the title as well?

seluianova

So, I tried to test as much as possible and noted some minor findings. Maybe I missed something because it was a lot to test...
Anyway if something else comes up I believe it can be fixed later.
Thanks for tackling this huge task 👍

💭 I think it would be nice to have a default text style (bodyMedium, for example), that doesn't need to be set explicitly to each text

UPD. ok, I see the discussion in the beginning about that

f1sh1918 · 2024-11-20T15:50:11Z

So, I tried to test as much as possible and noted some minor findings. Maybe I missed something because it was a lot to test... Anyway if something else comes up I believe it can be fixed later. Thanks for tackling this huge task 👍

💭 I think it would be nice to have a default text style (bodyMedium, for example), that doesn't need to be set explicitly to each text

UPD. ok, I see the discussion in the beginning about that

I believe the Scaffold widget has default body medium as michael mentioned and thats often a root widget. but you can create a task and we can add some default text widgets if you think its still needed. Thx for testing

codescene-delta-analysis

✅ Code Health Quality Gates: OK

Change in average Code Health of affected files: +0.00 (9.67 -> 9.67)

View detailed results in CodeScene

* 1696: add styles to texts and text buttons * 1696: move basic widget styles to theme, create typography class variables, removed unneeded variables * 1696: readd const * 1696: applied text color on tiles and dialog

1696: add styles to texts and text buttons

ca0da92

f1sh1918 requested review from steffenkleinle, ztefanie and seluianova as code owners October 30, 2024 15:49

ztefanie mentioned this pull request Nov 6, 2024

Update screenshots #1698

Closed

f1sh1918 added 2 commits November 12, 2024 14:07

1696: move basic widget styles to theme, create typography class vari…

b2e64b1

…ables, removed unneeded variables

Merge remote-tracking branch 'origin/main' into 1696-apply-fonts

ac7dcd4

# Conflicts: # frontend/lib/identification/util/activate_card.dart

1696: readd const

90fec31

ztefanie previously approved these changes Nov 19, 2024

View reviewed changes

ztefanie approved these changes Nov 19, 2024

View reviewed changes

seluianova reviewed Nov 19, 2024

View reviewed changes

frontend/lib/themes.dart Show resolved Hide resolved

seluianova reviewed Nov 19, 2024

View reviewed changes

seluianova mentioned this pull request Nov 19, 2024

1430: Extend card #1772

Merged

f1sh1918 requested a review from seluianova November 20, 2024 12:10

michael-markl reviewed Nov 20, 2024

View reviewed changes

seluianova reviewed Nov 20, 2024

View reviewed changes

seluianova approved these changes Nov 20, 2024

View reviewed changes

1696: applied text color on tiles and dialog

c820f44

f1sh1918 force-pushed the 1696-apply-fonts branch from c0d487a to c820f44 Compare November 20, 2024 15:59

f1sh1918 enabled auto-merge (squash) November 20, 2024 16:00

codescene-delta-analysis bot approved these changes Nov 20, 2024

View reviewed changes

f1sh1918 merged commit 0fe8ea4 into main Nov 20, 2024
2 checks passed

f1sh1918 deleted the 1696-apply-fonts branch November 20, 2024 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1696: Apply fonts #1728

1696: Apply fonts #1728

f1sh1918 commented Oct 30, 2024 •

edited

Loading

michael-markl commented Oct 30, 2024

f1sh1918 commented Oct 30, 2024 •

edited

Loading

michael-markl commented Nov 3, 2024 •

edited

Loading

f1sh1918 commented Nov 3, 2024

michael-markl commented Nov 3, 2024

f1sh1918 commented Nov 5, 2024 •

edited

Loading

michael-markl commented Nov 8, 2024 •

edited

Loading

f1sh1918 commented Nov 12, 2024 •

edited

Loading

f1sh1918 commented Nov 12, 2024

michael-markl commented Nov 18, 2024

ztefanie left a comment •

edited

Loading

ztefanie left a comment

seluianova left a comment •

edited

Loading

seluianova Nov 19, 2024 •

edited

Loading

f1sh1918 Nov 19, 2024

seluianova Nov 19, 2024

seluianova Nov 20, 2024 •

edited

Loading

f1sh1918 Nov 20, 2024 •

edited

Loading

seluianova Nov 20, 2024

seluianova Nov 19, 2024 •

edited

Loading

f1sh1918 Nov 20, 2024

seluianova Nov 20, 2024

f1sh1918 commented Nov 20, 2024

michael-markl Nov 1, 2024

michael-markl Nov 1, 2024

f1sh1918 Nov 20, 2024

michael-markl Nov 1, 2024

seluianova Nov 20, 2024 •

edited

Loading

seluianova left a comment •

edited

Loading

f1sh1918 commented Nov 20, 2024

codescene-delta-analysis bot left a comment

1696: Apply fonts #1728

1696: Apply fonts #1728

Conversation

f1sh1918 commented Oct 30, 2024 • edited Loading

Short description

Proposed changes

Side effects

Testing

Resolved issues

michael-markl commented Oct 30, 2024

f1sh1918 commented Oct 30, 2024 • edited Loading

michael-markl commented Nov 3, 2024 • edited Loading

f1sh1918 commented Nov 3, 2024

michael-markl commented Nov 3, 2024

f1sh1918 commented Nov 5, 2024 • edited Loading

michael-markl commented Nov 8, 2024 • edited Loading

f1sh1918 commented Nov 12, 2024 • edited Loading

f1sh1918 commented Nov 12, 2024

michael-markl commented Nov 18, 2024

ztefanie left a comment • edited Loading

Choose a reason for hiding this comment

ztefanie left a comment

Choose a reason for hiding this comment

seluianova left a comment • edited Loading

Choose a reason for hiding this comment

seluianova Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seluianova Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

f1sh1918 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seluianova Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1sh1918 commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seluianova Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

seluianova left a comment • edited Loading

Choose a reason for hiding this comment

f1sh1918 commented Nov 20, 2024

codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

f1sh1918 commented Oct 30, 2024 •

edited

Loading

f1sh1918 commented Oct 30, 2024 •

edited

Loading

michael-markl commented Nov 3, 2024 •

edited

Loading

f1sh1918 commented Nov 5, 2024 •

edited

Loading

michael-markl commented Nov 8, 2024 •

edited

Loading

f1sh1918 commented Nov 12, 2024 •

edited

Loading

ztefanie left a comment •

edited

Loading

seluianova left a comment •

edited

Loading

seluianova Nov 19, 2024 •

edited

Loading

seluianova Nov 20, 2024 •

edited

Loading

f1sh1918 Nov 20, 2024 •

edited

Loading

seluianova Nov 19, 2024 •

edited

Loading

seluianova Nov 20, 2024 •

edited

Loading

seluianova left a comment •

edited

Loading