Add "ChooseYourLookPage" #126

Feichtmeier · 2021-07-02T21:18:58Z

add two new assets from @waynecrosby for the theme previews
add AppTheme to main.dart and a new ChangeNotifierProvider for other Widgets to watch the theme change
add ChooseYourLookPage to the routes
add ChooseYourLookPage for the current design

TODO:

what's the next page to navigate to? @oSoMoN ? I left an "empty" function for the future - is this okay?
build the page via localisation

Closes #84
Closes #128

Thanks @jpnurmi for the help <3

@waynecrosby

- add two new assets from @waynecrosby for the theme previews - add AppTheme to main.dart and a new ChangeNotifierProvider for other Widgets to watch the theme change - add ChoseYourLookPage to the routes - add ChoseYourLookPage for the current design

jpnurmi

Should it be called 'Choose your look'?
I didn't notice any changes to pubspec.yaml. Is gsettings missing?
It would be nice to generate MockGSettings with mockito and have simple tests that AppTheme.apply() calls the right methods with correct arguments. Could even go one step further and add a widget test that presses the theme buttons.

packages/ubuntu_desktop_installer/lib/main.dart

jpnurmi · 2021-07-03T08:28:26Z

One more: test/widget_test.dart fails because it creates an instance of the whole UbuntuDesktopInstallerApp which requires a provider for AppTheme.

P.S. This one is actually a good example of why it's nice to have AppTheme somewhere else than in main.dart, because one would have to import the installer's main.dart with a main() function into the test file which has another instance of main(). Even though it works because the latter silently overrides the former, it seems more appropriate to not have two main() functions in the same program. 🙂

Feichtmeier · 2021-07-03T09:05:25Z

One more: test/widget_test.dart fails because it creates an instance of the whole UbuntuDesktopInstallerApp which requires a provider for AppTheme.

P.S. This one is actually a good example of why it's nice to have AppTheme somewhere else than in main.dart, because one would have to import the installer's main.dart with a main() function into the test file which has another instance of main(). Even though it works because the latter silently overrides the former, it seems more appropriate to not have two main() functions in the same program. slightly_smiling_face

I don't really understand this one. I need to create a test for the CI to pass? AppTheme is in a sep. file now :)

- build from OptionCard instead of radios - Correct the name to "Choose ..." - add localisation - add a subtitle

jpnurmi · 2021-07-03T10:16:45Z

I don't really understand this one.

In widget_test.dart, import app_theme.dart and add AppTheme in the list of the providers.

I need to create a test for the CI to pass?

Yes, it's part of the task. There's a check that the test coverage doesn't reduce significantly. :)

AppTheme is in a sep. file now :)

👍

codecov · 2021-07-03T10:25:00Z

Codecov Report

Merging #126 (34c3183) into main (d603fdc) will increase coverage by 1.52%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
+ Coverage   68.95%   70.48%   +1.52%     
==========================================
  Files          25       27       +2     
  Lines        1047     1108      +61     
==========================================
+ Hits          722      781      +59     
- Misses        325      327       +2

Impacted Files	Coverage Δ
packages/ubuntu_desktop_installer/lib/app.dart	`76.00% <50.00%> (-2.27%)`	⬇️
...top_installer/lib/pages/choose_your_look_page.dart	`96.77% <96.77%> (ø)`
...ckages/ubuntu_desktop_installer/lib/app_theme.dart	`100.00% <100.00%> (ø)`
...nstaller/lib/pages/write_changes_to_disk_page.dart	`84.84% <100.00%> (+0.11%)`	⬆️
...ntu_desktop_installer/lib/widgets/option_card.dart	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d603fdc...34c3183. Read the comment docs.

Feichtmeier · 2021-07-03T12:12:03Z

@jpnurmi added a test for the theme toggling, now the test fails on GitHub even if it passes locally 🤔

packages/ubuntu_desktop_installer/test/choose_your_look_page_test.dart

jpnurmi · 2021-07-03T12:15:29Z

P.S. In case anyone wonder's about the gsettings.dart dependency, I'm happy to transfer the package to Canonical later. ;)

jpnurmi · 2021-07-03T12:20:41Z

@jpnurmi added a test for the theme toggling, now the test fails on GitHub even if it passes locally

Probably missing some dependencies (e.g. DBUS daemon running?) and would probably even need to be run via Xvfb. I'd suggest leaving that test out for now until we have a GSettings mock available. It's not desired to mess with the system theme in tests anyway. 🙂

Feichtmeier · 2021-07-03T12:27:11Z

@jpnurmi added a test for the theme toggling, now the test fails on GitHub even if it passes locally

Probably missing some dependencies (e.g. DBUS daemon running?) and would probably even need to be run via Xvfb. I'd suggest leaving that test out for now until we have a GSettings mock available. It's not desired to mess with the system theme in tests anyway. slightly_smiling_face

okay so just revert back to the previous "count all the widgets" test?

MockGSettings can be used to verify GSettings calls without messing with system settings. Ref: #126

Because a plain mock is enough for this test... ;-) Ref: #126

Test and verify that when a theme is applied, the appropriate GSettings methods are called with correct arguments and in correct order. Ref: #126

- Mock AppTheme because in this test we only care that the right theme method is called, but not what AppTheme does internally. AppTheme has its own tests to verify that it does the right thing. ;) - Test the actual functionality of the page by tapping the appropriate OptionCards, which are searched by using localized strings so that the test continues to work even if the text is changed later. Also, this approach doesn't care how many text labels there are, because UI design changes tend to come and go while the app is still under heavy development. Ref: #126

jpnurmi · 2021-07-03T16:08:44Z

@Feichtmeier, I pushed some mocked test proposals to my fork. The changes are referenced above and there are explanations in each commit message. What do you think?

Feichtmeier · 2021-07-03T16:16:38Z

Looks nice - mind pushing this to this branch?

jpnurmi · 2021-07-03T16:22:54Z

I can't push to your branch, but you can try to pull the changes: git pull https://github.com/jpnurmi/ubuntu-desktop-installer.git Chose_your_look

Feichtmeier · 2021-07-03T16:40:29Z

@jpnurmi worked 👍 Thank you!

jpnurmi · 2021-07-03T16:44:08Z

I hope you don't mind the last change which is somewhat opinionated. Creating a page instance and counting widgets gives nice coverage results, but in the end, it doesn't test much. 🙃

Testing the functionality without making too many assumptions on the UI layout gives confidence that the page does the right thing, but more freedom for the design to evolve without needing to update tests because of minor UI tweaks. 😉

- WizardPage: reduce AppBar elevation to 1.0 to make it flat - OpionCard: change selected state to be flat, with accent color text and border

Feichtmeier · 2021-07-06T09:01:12Z

Updated OptionCard and WizardPage according to the discussion here: #84 (comment)

oSoMoN · 2021-07-06T10:37:30Z

what's the next page to navigate to? @oSoMoN ? I left an "empty" function for the future - is this okay?

Yes, this is okay for now, until we implement more pages. Could you please link the current last page ("allocate disk space") to this new page, so when pressing "Start Installing" this new page is displayed? This will ease manual testing, and the order of pages can be re-shuffled later on.

jpnurmi

P.S. The appbar and option card changes could be even a separate PR. It would be quick to review and those changes are not strictly related to this PR anyway. :)

packages/ubuntu_desktop_installer/lib/routes.dart

oSoMoN · 2021-07-06T13:10:48Z

P.S. The appbar and option card changes could be even a separate PR. It would be quick to review and those changes are not strictly related to this PR anyway. :)

Seconded!

This reverts commit aa61ad2.

Feichtmeier · 2021-07-06T19:11:11Z

what's the next page to navigate to? @oSoMoN ? I left an "empty" function for the future - is this okay?

Yes, this is okay for now, until we implement more pages. Could you please link the current last page ("allocate disk space") to this new page, so when pressing "Start Installing" this new page is displayed? This will ease manual testing, and the order of pages can be re-shuffled later on.

@oSoMoN sorry to ask one more time, but I did not really understand you here.

Currently AllocateDiskSpacePage navigates to nothing in my branch, I guess it is outdated?
Can I just skip this step then for this PR? :)

Never mind, my branch was outdated

…staller into Chose_your_look

- navigate to ChooseYourLookPage

Feichtmeier · 2021-07-07T09:09:32Z

@oSoMoN I think this is what you meant, right? :)

oSoMoN · 2021-07-07T09:39:52Z

@oSoMoN I think this is what you meant, right? :)

Yes, indeed.

Feichtmeier · 2021-07-07T12:03:57Z

@oSoMoN your requested change now conflicts with the recent commit in oSoMoN@15b62ba

Shall I again overwrite it as requested or change lines to what is in master now?

oSoMoN · 2021-07-07T13:08:16Z

@oSoMoN your requested change now conflicts with the recent commit in oSoMoN@15b62ba

Yes, sorry about that. Can you please rebase on master, and add your page to the end of the current last page ("Write changes to disk")?

…staller into Chose_your_look

- navigate to ChooseYourLookPage on continute action

Feichtmeier · 2021-07-07T13:17:17Z

@oSoMoN your requested change now conflicts with the recent commit in oSoMoN@15b62ba

Yes, sorry about that. Can you please rebase on master, and add your page to the end of the current last page ("Write changes to disk")?

Done 👍 - I hope merging upstream main into my branch was also okay? I guess you squash and merge anyways, right? :)

packages/ubuntu_desktop_installer/lib/routes.dart

oSoMoN · 2021-07-07T15:33:44Z

LGTM, thanks!

Feichtmeier added 2 commits July 2, 2021 23:18

Add "ChoseYourLookPage"

06cbf4e

- add two new assets from @waynecrosby for the theme previews - add AppTheme to main.dart and a new ChangeNotifierProvider for other Widgets to watch the theme change - add ChoseYourLookPage to the routes - add ChoseYourLookPage for the current design

Add localisation

ee88b19

jpnurmi reviewed Jul 3, 2021

View reviewed changes

packages/ubuntu_desktop_installer/lib/main.dart Outdated Show resolved Hide resolved

Feichtmeier added 2 commits July 3, 2021 10:24

add gsettings to pubspec

365dc6e

Move AppTheme to its own file

ab080b7

Feichtmeier added 3 commits July 3, 2021 11:29

ChooseYourLookPage improvements

a8c9558

- build from OptionCard instead of radios - Correct the name to "Choose ..." - add localisation - add a subtitle

Use the correct header text

bf85119

Change homepage pack to WelcomePage

497eb06

Feichtmeier changed the title ~~Add "ChoseYourLookPage"~~ Add "ChooseYourLookPage" Jul 3, 2021

Add AppTheme to the widget_test providers

7e0acf9

Add choose_your_look_page_test

8ddc282

Feichtmeier marked this pull request as ready for review July 3, 2021 11:07

Add theme tests to the test

3f1c1a0

jpnurmi reviewed Jul 3, 2021

View reviewed changes

packages/ubuntu_desktop_installer/test/choose_your_look_page_test.dart Outdated Show resolved Hide resolved

Feichtmeier and others added 5 commits July 3, 2021 14:53

Add padding and simplify the test

5b90501

Setup GSettings mock

5936261

MockGSettings can be used to verify GSettings calls without messing with system settings. Ref: #126

Use MockGSettings in widget_test

e3380c6

Because a plain mock is enough for this test... ;-) Ref: #126

Test AppTheme

ea01d45

Test and verify that when a theme is applied, the appropriate GSettings methods are called with correct arguments and in correct order. Ref: #126

OptionCard and WizardPage change

aa61ad2

- WizardPage: reduce AppBar elevation to 1.0 to make it flat - OpionCard: change selected state to be flat, with accent color text and border

jpnurmi reviewed Jul 6, 2021

View reviewed changes

packages/ubuntu_desktop_installer/lib/routes.dart Outdated Show resolved Hide resolved

Feichtmeier added 2 commits July 6, 2021 20:49

Revert "OptionCard and WizardPage change"

243884b

This reverts commit aa61ad2.

Fix typo

90268a4

Feichtmeier added 2 commits July 6, 2021 21:23

Merge branch 'main' of https://github.com/canonical/ubuntu-desktop-in…

03ff153

…staller into Chose_your_look

AllocateDiskSpacePage: navigation change

3522074

- navigate to ChooseYourLookPage

Feichtmeier added 2 commits July 7, 2021 15:13

Merge branch 'main' of https://github.com/canonical/ubuntu-desktop-in…

f150b54

…staller into Chose_your_look

WriteChangesToDiskPage: Navigation change

8ac7392

- navigate to ChooseYourLookPage on continute action

oSoMoN suggested changes Jul 7, 2021

View reviewed changes

packages/ubuntu_desktop_installer/lib/routes.dart Outdated Show resolved Hide resolved

Feichtmeier and others added 2 commits July 7, 2021 16:47

ChooseYourLookPage: add missing slash in route

6d37014

Fix a widget test.

34c3183

oSoMoN approved these changes Jul 7, 2021

View reviewed changes

oSoMoN merged commit a290e16 into canonical:main Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "ChooseYourLookPage" #126

Add "ChooseYourLookPage" #126

Feichtmeier commented Jul 2, 2021 •

edited

Loading

jpnurmi left a comment

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

codecov bot commented Jul 3, 2021 •

edited

Loading

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 6, 2021

oSoMoN commented Jul 6, 2021

jpnurmi left a comment

oSoMoN commented Jul 6, 2021

Feichtmeier commented Jul 6, 2021 •

edited

Loading

Feichtmeier commented Jul 7, 2021

oSoMoN commented Jul 7, 2021

Feichtmeier commented Jul 7, 2021

oSoMoN commented Jul 7, 2021

Feichtmeier commented Jul 7, 2021

oSoMoN commented Jul 7, 2021

Add "ChooseYourLookPage" #126

Add "ChooseYourLookPage" #126

Conversation

Feichtmeier commented Jul 2, 2021 • edited Loading

jpnurmi left a comment

Choose a reason for hiding this comment

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

codecov bot commented Jul 3, 2021 • edited Loading

Codecov Report

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 3, 2021

jpnurmi commented Jul 3, 2021

Feichtmeier commented Jul 6, 2021

oSoMoN commented Jul 6, 2021

jpnurmi left a comment

Choose a reason for hiding this comment

oSoMoN commented Jul 6, 2021

Feichtmeier commented Jul 6, 2021 • edited Loading

Feichtmeier commented Jul 7, 2021

oSoMoN commented Jul 7, 2021

Feichtmeier commented Jul 7, 2021

oSoMoN commented Jul 7, 2021

Feichtmeier commented Jul 7, 2021

oSoMoN commented Jul 7, 2021

Feichtmeier commented Jul 2, 2021 •

edited

Loading

codecov bot commented Jul 3, 2021 •

edited

Loading

Feichtmeier commented Jul 6, 2021 •

edited

Loading