Skip to content

Add Density API to ThemeData, implement for buttons. #43547

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

Merged
merged 3 commits into from
Dec 4, 2019

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Oct 25, 2019

Description

This implements a draft API for specifying the concept of VisualDensity for a Material user interface. This will allow us to specify default densities for desktop platforms that are more reasonable, since desktop platforms don't have the same constraints around tap target sizes.

This PR adds the new VisualDensity class, and an attribute on the ThemeData and ButtonThemeData classes for setting it. I applied this new theme information to the buttons which use RawMaterialButton. The implementation of the density changes is in RawMaterialButton.

Tests

  • Added tests for each of the buttons, and for ThemeData.

Breaking Change

  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Oct 25, 2019
@gspencergoog gspencergoog force-pushed the density branch 6 times, most recently from d9ad0f6 to ea22eb1 Compare October 29, 2019 20:56
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking this looks fine. A few overall concerns though:

  • Should we give Density a less generic name? VisualDensity maybe? It's possible that "Density" will collide with an existing name in some apps.

  • It might be best to leave density out of ButtonTheme, and all the rest of the component themes moving forward. At least for now. ButtonTheme includes a colorScheme parameter, which was probably a mistake. Most of the other component themes do not provide properties that shadow generic theme parameters like textTheme or primaryColor. Similarly, specifying density per component also seems like it might be overdoing it, at least at the outset.

  • Prefer a gallery demo in a separate PR to a manual test.

@gspencergoog
Copy link
Contributor Author

  • Should we give Density a less generic name? VisualDensity maybe? It's possible that "Density" will collide with an existing name in some apps.

Sure. I like VisualDensity. Would UIDensity work as well?

  • It might be best to leave density out of ButtonTheme, and all the rest of the component themes moving forward. At least for now. ButtonTheme includes a colorScheme parameter, which was probably a mistake. Most of the other component themes do not provide properties that shadow generic theme parameters like textTheme or primaryColor. Similarly, specifying density per component also seems like it might be overdoing it, at least at the outset.

OK, I'll remove it from ButtonTheme and have the button only look at the ThemeData, and remove it from the per component args. It will make it harder to set per component, but that might not be too common.

  • Prefer a gallery demo in a separate PR to a manual test.

Actually, I don't think the manual test belongs in the gallery. I mean, there should be a gallery demo for density too (probably just a setting in the overall settings page), but this manual test is really useful as a separate app.

I wrote it because it allows me to take a single component and put it under manual testing where I can actually see it, trying it out in a variety of conditions so that I can quickly see how it behaves under odd conditions that I don't think we want to duplicate in the gallery app. It's also a lot smaller when I have to debug something and look at the widget tree.

For different components, both Darren and I have independently invented similar apps to do what this app does, and it seemed inefficient to keep re-inventing it.

My hope is that these will proliferate, and we'll end up with an easy way to boot up any component and twist the dials and see what happens.

I'm happy to put it into another PR, of course.

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Nov 22, 2019

@HansMuller and I talked about this a bit more, and arrived at having a "profile" on the VisualDensity that can be interpreted by the individual components so that they can adjust their own interpretation based on the profile.

For particular controls, it would adjust based on deltas from the spec instead of using the absolute spec value. So, for instance, if the spec says that for the "comfortable" profile an OutlineButton is density -1, when the base density is -2, then an OutlineButton given a density with a profile of "comfortable" would add one to the density set in the VisualDensity object.

And, I implemented it here, with OutlineButton serving as an example of how customization could be done to see how it works.

It works OK, but there is a problem... you can't lerp an enum, so it blows our ability to lerp smoothly between two "profiles". Sadness.

@gspencergoog
Copy link
Contributor Author

I think the best thing now might to just rip out the profile stuff entirely, and just associate numbers with the profile names (set up static doubles for the profile types, so VisualDensity.comfortable might be -1.0, and VisualDensity.compact might be -2.0), and have the components that differ from the norm just have some kind of curve that defines how they map the base density to their own density adjustments. That keeps the complexity out of the VisualDensity object, is infinitely flexible still, and can still be lerped. The downside is that it makes the adjustments at the component level much more complex, but at least that complexity is hidden.

@gspencergoog gspencergoog force-pushed the density branch 3 times, most recently from e877330 to 2bbf89e Compare November 22, 2019 21:59
@gspencergoog gspencergoog force-pushed the density branch 2 times, most recently from 7768637 to 9cdfd7d Compare November 25, 2019 16:16
@gspencergoog gspencergoog force-pushed the density branch 2 times, most recently from b3ff725 to 3546f75 Compare December 3, 2019 19:32
@gspencergoog gspencergoog marked this pull request as ready for review December 3, 2019 19:56
@gspencergoog gspencergoog changed the title WIP: Add Density API to ThemeData, implement for buttons. Add Density API to ThemeData, implement for buttons. Dec 3, 2019
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gspencergoog gspencergoog merged commit 185da9b into flutter:master Dec 4, 2019
@gspencergoog gspencergoog deleted the density branch December 4, 2019 15:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: contributor-productivity Team-specific productivity, code health, technical debt. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants