Skip to content
This repository has been archived by the owner on Mar 6, 2024. It is now read-only.

ChooseYourLook: use yaru_widgets.dart for images #656

Merged
merged 7 commits into from
Feb 7, 2022
Merged

ChooseYourLook: use yaru_widgets.dart for images #656

merged 7 commits into from
Feb 7, 2022

Conversation

Feichtmeier
Copy link
Contributor

@Feichtmeier Feichtmeier commented Feb 1, 2022

Before After with 1/3 of the screenwidth After with 1/4 of the screen width
grafik grafik grafik

CC @jpnurmi @elioqoshi please have a look concerning the size of the images
I convert the PR to draft because I realized that I use the Yaru* suffix for all widgets in yaru_widgets.dart ... except the ImageTile (fixed)

Closes #483

@Feichtmeier Feichtmeier marked this pull request as draft February 1, 2022 20:28
@Feichtmeier Feichtmeier marked this pull request as ready for review February 1, 2022 20:44
Copy link
Contributor

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

Thanks, @Feichtmeier! It's very nice!

@elioqoshi, which one do you like better; the smaller or larger thumbnails?

@Muqtxdir
Copy link

Muqtxdir commented Feb 2, 2022

@Feichtmeier could you please use the svgs from this branch: https://gitlab.gnome.org/Community/Ubuntu/gnome-control-center/-/merge_requests/5

@Feichtmeier
Copy link
Contributor Author

@Muqtxdir thanks I will!

I am currently implementing @jpnurmi 's suggestion to wrap a Widget rather than always an Image

For the future we could stick to the svg format for assets because they can be easily recolored by flutter svg

@elioqoshi
Copy link

1/3 of the screenwidth looks best, thanks @Feichtmeier !

In case we will add a 3rd option in the future (Automatic?) we can adjust the sizes again I guess.

@jpnurmi
Copy link
Contributor

jpnurmi commented Feb 2, 2022

@Feichtmeier Here are the required test changes with YaruImageTile (needs adapting to YaruSelectionTile once available)

diff --git a/packages/ubuntu_desktop_installer/test/choose_your_look/choose_your_look_page_test.dart b/packages/ubuntu_desktop_installer/test/choose_your_look/choose_your_look_page_tes>
index 2f5fff0e..915a422e 100644
--- a/packages/ubuntu_desktop_installer/test/choose_your_look/choose_your_look_page_test.dart
+++ b/packages/ubuntu_desktop_installer/test/choose_your_look/choose_your_look_page_test.dart
@@ -7,6 +7,7 @@ import 'package:ubuntu_desktop_installer/l10n.dart';
 import 'package:ubuntu_desktop_installer/pages/choose_your_look_page.dart';
 import 'package:ubuntu_wizard/settings.dart';
 import 'package:ubuntu_wizard/widgets.dart';
+import 'package:yaru_widgets/yaru_widgets.dart';
 
 import '../widget_tester_extensions.dart';
 import 'choose_your_look_page_test.mocks.dart';
@@ -28,20 +29,20 @@ void main() {
       ),
     );
 
-    final lightOptionCard = find.widgetWithText(
-      OptionCard,
-      lang(tester).chooseYourLookPageLightSetting,
+    final lightImageTile = find.widgetWithImage(
+      YaruImageTile,
+      AssetImage('assets/Theme_thumbnails-Light.png'),
     );
     expect(lightImageTile, findsOneWidget);
     await tester.tap(lightImageTile);
     verify(settings.applyTheme(Brightness.light));
 
-    final darkOptionCard = find.widgetWithText(
-      OptionCard,
-      lang(tester).chooseYourLookPageDarkSetting,
+    final darkImageTile = find.widgetWithImage(
+      YaruImageTile,
+      AssetImage('assets/Theme_thumbnails-Dark.png'),
     );
-    expect(darkOptionCard, findsOneWidget);
-    await tester.tap(darkOptionCard);
+    expect(darkImageTile, findsOneWidget);
+    await tester.tap(darkImageTile);
     verify(settings.applyTheme(Brightness.dark));
   });
 }

@Feichtmeier
Copy link
Contributor Author

Thanks @jpnurmi - I've included your suggested changes and the test. But I've decided to extract those Columns to a private Widget to make the page more readable, hope this was okay?

@Feichtmeier
Copy link
Contributor Author

Feichtmeier commented Feb 2, 2022

@Feichtmeier could you please use the svgs from this branch: https://gitlab.gnome.org/Community/Ubuntu/gnome-control-center/-/merge_requests/5

Nice updated pictures!
However could we eventually make those previews even more abstract? I think we do not need to draw the buttons in the headerbar, etc.

@Feichtmeier
Copy link
Contributor Author

1/3 of the screenwidth looks best, thanks @Feichtmeier !

In case we will add a 3rd option in the future (Automatic?) we can adjust the sizes again I guess.

yes :) Here is a faked third option (wrong image)
grafik
we only need to adapt the width to screenwidth / 4 for 3 cards, etc

@jpnurmi
Copy link
Contributor

jpnurmi commented Feb 7, 2022

For reference, the final looks with the currently available options:
Screenshot from 2022-02-07 12-02-00

Copy link
Contributor

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

Thank you @Feichtmeier! LGTM 👍

@jpnurmi jpnurmi merged commit e547c00 into canonical:main Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should the theme selection titles be changed?
4 participants