Skip to content

Add visual density to the gallery options#46090

Merged
gspencergoog merged 6 commits intoflutter:masterfrom
gspencergoog:gallery_density
Dec 21, 2019
Merged

Add visual density to the gallery options#46090
gspencergoog merged 6 commits intoflutter:masterfrom
gspencergoog:gallery_density

Conversation

@gspencergoog
Copy link
Copy Markdown
Contributor

Description

This PR adds an option to the gallery options that sets the visual density to one of "standard" (the default), "comfortable", or "compact".

Tests

  • Added a test to exercise the new option.

Breaking Change

  • No, this is not a breaking change.

@fluttergithubbot fluttergithubbot added d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. labels Dec 4, 2019
@shihaohong shihaohong self-requested a review December 4, 2019 17:50
Comment thread packages/flutter/test/material/raw_material_button_test.dart Outdated
Copy link
Copy Markdown
Contributor

@shihaohong shihaohong Dec 4, 2019

Choose a reason for hiding this comment

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

Is there a way to particularly target this instance of Text('System Default') here and elsewhere? I was thinking something like:

  find.descendant(
    of: find.byType(PopupMenu),
    matching: find.text('System Default')
  )

However, it seems both _PopupMenu and the _VisualDensityItem widgets are private, so I'm not sure if it's possible, but that would improve the process of updating these tests and prevent future bugs/confusion

Copy link
Copy Markdown
Contributor Author

@gspencergoog gspencergoog Dec 18, 2019

Choose a reason for hiding this comment

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

It's possible, it looks like this:

find.descendant(
  of: find.byWidgetPredicate((Widget widget) => widget.runtimeType.toString() == '_PopupMenu'),
  matching: find.text('System Default')
)

I switched to something like this, but I used 'PopupMenuItem' because the byType finder can't find specialized generics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's pretty neat, I'll definitely keep this in mind when I need it in the future!

@gspencergoog
Copy link
Copy Markdown
Contributor Author

I think this is ready for another look.

Copy link
Copy Markdown
Contributor

@shihaohong shihaohong 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 924efae into flutter:master Dec 21, 2019
@flar
Copy link
Copy Markdown
Contributor

flar commented Dec 21, 2019

This PR failed the analyzer on linux:

▌04:44:01▐ RUNNING: cd .; bin/flutter analyze --dartdocs --flutter-repo
Analyzing 3 directories...                                      
  error • A value of type 'dynamic' can't be assigned to a variable of type 'GalleryVisualDensityValue' • examples/flutter_gallery/lib/gallery/scales.dart:50:50 • invalid_assignment
  error • A value of type 'Widget' can't be assigned to a variable of type 'MaterialApp' • examples/flutter_gallery/test/drawer_test.dart:73:11 • invalid_assignment
  error • A value of type 'Widget' can't be assigned to a variable of type 'MaterialApp' • examples/flutter_gallery/test/drawer_test.dart:83:11 • invalid_assignment
3 issues found. (ran in 278.8s)
▌04:48:43▐ ELAPSED TIME: 4min 41.944s for bin/flutter analyze --dartdocs --flutter-repo in .

@franciscojma86
Copy link
Copy Markdown
Contributor

@flar why wasn't this reverted in the end? Just curious since seems like it might have introduced some performance regressions so not sure if it might be related.

@flar
Copy link
Copy Markdown
Contributor

flar commented Dec 23, 2019

@franciscojma86
The build failures were fixed by someone else before I merged: #47594

@franciscojma86
Copy link
Copy Markdown
Contributor

I see, thanks!

franciscojma86 added a commit to franciscojma86/flutter that referenced this pull request Dec 23, 2019
franciscojma86 added a commit that referenced this pull request Dec 23, 2019
* Revert "Add visual density to the gallery options (#46090)"

This reverts commit 924efae.

* Revert "fix analysis (#47594)"

This reverts commit 49c7845.
franciscojma86 added a commit to franciscojma86/flutter that referenced this pull request Jan 2, 2020
@flar
Copy link
Copy Markdown
Contributor

flar commented Jan 11, 2020

@flar why wasn't this reverted in the end? Just curious since seems like it might have introduced some performance regressions so not sure if it might be related.

I'm just seeing some performance regressions from around this time so you may be right, but they are subtle enough and the benchmarks themselves are noisy enough that it is hard to point at a specific commit that could have been the cause.

@gspencergoog gspencergoog deleted the gallery_density branch January 16, 2020 22:10
PK2702 added a commit to PK2702/flutter that referenced this pull request Jan 23, 2020
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 2, 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. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. d: examples Sample code and demos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants