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

Proposal : MaterialApp - Make MediaQuery be available while constructing app Theme #80931

Open
zs-dima opened this issue Apr 22, 2021 · 13 comments · Fixed by #81295
Open

Proposal : MaterialApp - Make MediaQuery be available while constructing app Theme #80931

zs-dima opened this issue Apr 22, 2021 · 13 comments · Fixed by #81295
Labels
a: quality A truly polished experience c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team-design Owned by Design Languages team triaged-design Triaged by Design Languages team

Comments

@zs-dima
Copy link

zs-dima commented Apr 22, 2021

MaterialApp theme builder have no possibility to use MediaQuery - so it could not be used in real-life apps without ugly workarounds.
App themes depends on current screen width / system theme type and etc.
I found workarounds that have own issues:

1.

MaterialApp(
        home: child,
        builder: (context, child) => AnimatedTheme(
          data: appTheme(context),
          child: child!,

but it duplicate AnimatedTheme(data: ThemeData.light() creation in the MaterialApp

2.

MediaQuery(
  data: MediaQueryData.fromWindow(window),
  child: MaterialApp(
        home: child,
        theme: appTheme(context),

but it duplicate MediaQuery creation (or alternative widget as sample above has no MediaQuery changes notifications for the theme) in the MaterialApp
Is there any simple way to disable duplicate AnimatedTheme creation for 1.? Could it be MaterialApp parameter or etc?

@zs-dima zs-dima changed the title MaterialApp have no possibility to use context MaterialApp theme have no possibility to use context Apr 22, 2021
@zs-dima zs-dima changed the title MaterialApp theme have no possibility to use context MaterialApp - could not construct Theme based on MediaQuery Apr 22, 2021
@darshankawar darshankawar added the in triage Presently being triaged by the triage team label Apr 22, 2021
@darshankawar
Copy link
Member

@zs-dima
If I understand your issue correctly, you are unable to use MediaQuery.of(context) for Themes ?

@darshankawar darshankawar added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 22, 2021
@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

@darshankawar thanks for reply

Yes, it is not possible to set theme fonts sizes depends on MediaQuery.of(context).size.width and etc without workarounds I mention above.
But this workarounds have issues I mention.
Looks MaterialApp could be improved to be able to work with themes correctly. Maybe add parameter to do not wrap child by additional AnimatedTheme internally.

@no-response no-response bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 22, 2021
@darshankawar
Copy link
Member

@zs-dima
Can you check out this SO link and see if it helps to answer your query / issue ?

@darshankawar darshankawar added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 22, 2021
@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

@darshankawar sure it is not.
I looked into MaterialApp source code and could see it force wrap child in the unneeded AnimatedTheme(data: ThemeData.light()
Simple MaterialApp fix could be made child wrapping optional.

@no-response no-response bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 22, 2021
@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

So MaterialApp force us to set theme as MaterialApp(theme: appTheme(context), but on other hand it create MediaQuery below theme by mistake - so we could not create theme based on actual device screen size and etc.

@darshankawar
Copy link
Member

@zs-dima
Thanks for digging in. What's your ask / proposal here to fix this limitation / behavior ?

@darshankawar darshankawar added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 22, 2021
@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

My proposal to make MediaQuery be available while we constructing app Theme.
So we could construct app Theme depends on device details and system settings MediaQueryData contains.
And we could update Theme when device details changes (MediaQuery.of() behave in this way already)

Simplest solution from my point of view is to make optional wrapping MaterialApp child with AnimatedTheme internally.
Then we will be able to use MaterialApp(builder: (context, child) => AnimatedTheme( without additional wrapping.

Other solution could be to move MediaQuery over theme construction - then it will be MaterialApp(themeBuilder: (context) => appTheme(context)

@no-response no-response bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 22, 2021
@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

@darshankawar thanks a lot for looking into the issue details and trying to solve it.

@AbhishekDoshi26
Copy link

I guess using MediaQuery.of(context) won't be possible in MaterialApp. This is because of(context) takes the context of parent widget. However MaterialApp is the root of widget tree and hence there is no Parent of it. I may be wrong in interpreting the behaviour of MaterialApp and of(context). What do you think @darshankawar ?

@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

@AbhishekDoshi26 therefore I suggesting to use themeBuilder or be able to disable force-wrap AnimatedTheme above.

@zs-dima
Copy link
Author

zs-dima commented Apr 22, 2021

root of widget tree

I mean MaterialApp internal sub-tree - it could be constructed in better way
Currently it is MaterialApp>AnimatedTheme>MediaQuery
But it could be MaterialApp>MediaQuery>AnimatedTheme at least

@HansMuller HansMuller added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 22, 2021
@darshankawar darshankawar added passed first triage c: proposal A detailed proposal for a change to Flutter c: new feature Nothing broken; request for a new capability a: quality A truly polished experience and removed in triage Presently being triaged by the triage team labels Apr 23, 2021
@darshankawar darshankawar changed the title MaterialApp - could not construct Theme based on MediaQuery Proposal : MaterialApp - Make MediaQuery be available while constructing app Theme Apr 23, 2021
@darshankawar darshankawar added the r: fixed Issue is closed as already fixed in a newer version label Jul 1, 2021
@zs-dima
Copy link
Author

zs-dima commented Jul 8, 2021

@darshankawar thanks a lot for looking into this issue

as I could not see changes are not in dev still

It going to be MediaQuery>MaterialApp>AnimatedTheme

however it could be nice to have MaterialApp>MediaQuery>AnimatedTheme ideally

@pedromassango
Copy link
Member

Reopening as the PR has been reverted (see #85223)

@pedromassango pedromassango reopened this Jul 21, 2021
@pedromassango pedromassango removed the r: fixed Issue is closed as already fixed in a newer version label Jul 21, 2021
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-design Owned by Design Languages team triaged-design Triaged by Design Languages team labels Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: quality A truly polished experience c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team-design Owned by Design Languages team triaged-design Triaged by Design Languages team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants