Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(938)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1204523002: Material light and dark themes for Sky widgets (Closed)

Created:
5 years, 6 months ago by jackson
Modified:
5 years, 6 months ago
CC:
abarth-chromium, gregsimon, jackson_old, mojo-reviews_chromium.org, qsr+mojo_chromium.org
Base URL:
git@github.com:domokit/mojo.git@master
Target Ref:
refs/heads/master
Project:
mojo
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : support accent colors #

Total comments: 7

Patch Set 3 : add a default theme #

Patch Set 4 : update tests #

Patch Set 5 : fix analyzer #

Patch Set 6 : rebase #

Patch Set 7 : fix analyzer warning properly #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+238 lines, -37 lines) Patch
M sky/examples/stocks2/lib/stock_app.dart View 1 2 chunks +4 lines, -2 lines 0 comments Download
M sky/examples/stocks2/lib/stock_home.dart View 2 chunks +1 line, -2 lines 0 comments Download
M sky/examples/stocks2/lib/stock_row.dart View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M sky/examples/widgets/sector.dart View 1 1 chunk +2 lines, -2 lines 0 comments Download
M sky/examples/widgets/styled_text.dart View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M sky/sdk/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M sky/sdk/lib/painting/text_style.dart View 1 2 3 4 2 chunks +15 lines, -1 line 0 comments Download
M sky/sdk/lib/theme/colors.dart View 1 2 3 17 chunks +113 lines, -0 lines 0 comments Download
M sky/sdk/lib/theme/theme_data.dart View 1 2 3 1 chunk +32 lines, -4 lines 0 comments Download
M sky/sdk/lib/widgets/basic.dart View 2 chunks +13 lines, -1 line 0 comments Download
M sky/sdk/lib/widgets/checkbox.dart View 1 chunk +1 line, -1 line 0 comments Download
A sky/sdk/lib/widgets/default_text_style.dart View 1 chunk +26 lines, -0 lines 0 comments Download
M sky/sdk/lib/widgets/floating_action_button.dart View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M sky/sdk/lib/widgets/material.dart View 1 2 3 4 2 chunks +4 lines, -3 lines 0 comments Download
M sky/sdk/lib/widgets/switch.dart View 2 chunks +7 lines, -6 lines 0 comments Download
M sky/sdk/lib/widgets/theme.dart View 1 2 1 chunk +3 lines, -3 lines 2 comments Download
M sky/sdk/lib/widgets/tool_bar.dart View 3 chunks +6 lines, -2 lines 0 comments Download
M sky/sdk/lib/widgets/widget.dart View 1 2 1 chunk +1 line, -1 line 0 comments Download
M sky/tests/examples/stocks-expected.txt View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M sky/tests/widgets/buttons-expected.txt View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 7 (1 generated)
eseidel
https://codereview.chromium.org/1204523002/diff/20001/sky/examples/stocks2/lib/stock_row.dart File sky/examples/stocks2/lib/stock_row.dart (right): https://codereview.chromium.org/1204523002/diff/20001/sky/examples/stocks2/lib/stock_row.dart#newcode47 sky/examples/stocks2/lib/stock_row.dart:47: style: Theme.of(this).text.caption.copyWith(textAlign: TextAlign.right) Should this be this.theme() instead?
5 years, 6 months ago (2015-06-22 22:28:17 UTC) #2
jackson
https://codereview.chromium.org/1204523002/diff/20001/sky/examples/stocks2/lib/stock_row.dart File sky/examples/stocks2/lib/stock_row.dart (right): https://codereview.chromium.org/1204523002/diff/20001/sky/examples/stocks2/lib/stock_row.dart#newcode47 sky/examples/stocks2/lib/stock_row.dart:47: style: Theme.of(this).text.caption.copyWith(textAlign: TextAlign.right) On 2015/06/22 22:28:16, eseidel wrote: > ...
5 years, 6 months ago (2015-06-22 22:40:10 UTC) #3
Hixie
lgtm this is amazing. https://codereview.chromium.org/1204523002/diff/20001/sky/sdk/lib/theme/theme_data.dart File sky/sdk/lib/theme/theme_data.dart (right): https://codereview.chromium.org/1204523002/diff/20001/sky/sdk/lib/theme/theme_data.dart#newcode7 sky/sdk/lib/theme/theme_data.dart:7: import 'typography.dart'; import this "as ...
5 years, 6 months ago (2015-06-22 23:08:41 UTC) #4
jackson
Committed patchset #7 (id:120001) manually as 6ade7dc07b50d4ac8df8c7abbb1a2e2515562fc3 (presubmit successful).
5 years, 6 months ago (2015-06-23 00:14:31 UTC) #5
abarth-chromium
https://codereview.chromium.org/1204523002/diff/120001/sky/sdk/lib/widgets/theme.dart File sky/sdk/lib/widgets/theme.dart (right): https://codereview.chromium.org/1204523002/diff/120001/sky/sdk/lib/widgets/theme.dart#newcode22 sky/sdk/lib/widgets/theme.dart:22: static ThemeData _kFallbackTheme = new ThemeData.fallback(); final
5 years, 6 months ago (2015-06-23 02:11:36 UTC) #6
jackson
5 years, 6 months ago (2015-06-23 17:14:10 UTC) #7
Message was sent while issue was closed. 
https://codereview.chromium.org/1204523002/diff/20001/sky/sdk/lib/theme/theme...
File sky/sdk/lib/theme/theme_data.dart (right):

https://codereview.chromium.org/1204523002/diff/20001/sky/sdk/lib/theme/theme...
sky/sdk/lib/theme/theme_data.dart:13: this.accent,
On 2015/06/22 23:08:41, Hixie wrote:
> we should either default primary and accent to something useful (if
arbitrary),
> or we should assert that they're not null.
> 
> I'd prefer defaulting.

Done.

https://codereview.chromium.org/1204523002/diff/120001/sky/sdk/lib/widgets/th...
File sky/sdk/lib/widgets/theme.dart (right):

https://codereview.chromium.org/1204523002/diff/120001/sky/sdk/lib/widgets/th...
sky/sdk/lib/widgets/theme.dart:22: static ThemeData _kFallbackTheme = new
ThemeData.fallback();
On 2015/06/23 02:11:36, abarth wrote:
> final

Fixed in https://codereview.chromium.org/1192773004

Powered by Google App Engine
This is Rietveld 408576698