settings_page - Githubissues

RIP-Comm / sossoldi

"Sossoldi" is a wealth management / personal finance / Net Worth tracking app, made with Flutter.

MIT License

276 stars 75 forks source link

settings_page #58

Closed AntonioMiclaus closed 1 year ago

AntonioMiclaus commented 1 year ago

I've added the settings_page with the four main options.

theperu commented 1 year ago

Great job! I tried this on both iOS & Android and the only thing that looks a bit strange to me is the upper part of the screen for two reasons:

Having a grey background seems strange and it is different from the other parts of the app, maybe I would change it to the same white used for the rest of the page. If the icon and the text are not easily visible we could set them to the same blue of the other ones.
"Back" is not right besides the icon (on iOS is shown in the middle in fact). If you struggle on moving it to the left a work around could be to remove the word "Back" leaving only the icon and having settings written in the same line in center instead of where is now

FedericoBruzzone commented 1 year ago

Can you send a picture? I can't test your code.

theperu commented 1 year ago

Here is how it looks on iOS & Android

FedericoBruzzone commented 1 year ago

I agree with what you said. 👍

AntonioMiclaus commented 1 year ago

I've updated the appbar by removing the "back" text and the grey background and added the text "Settings" in the center with also the dark blue back arrow.

theperu commented 1 year ago

Hi! Everything looks fine to me now. Just a little question, I saw that in your commits you also pushed changes to macos/Flutter/GeneratedPluginRegistrant.swift and pubspec.lock but it don't understand why. Can you help me with that?

AntonioMiclaus commented 1 year ago

I noticed these files changed automatically when I tested the code with VScode + android studio. The added parts may be requirements. The parts removed may not be needed anymore or maybe only for the device I tested on. The only file I changed manually is the settings_page.dart

theperu commented 1 year ago

Understood, I have the feeling that a PR like this should contain only the changes on settings_page.dart. Maybe @FedericoBruzzone or @mikev-cw can take a look since I am not that familiar with code reviews

mikev-cw commented 1 year ago

@theperu Yeah! Agreed. @AntonioMiclaus seems that files are still in the PR. Maybe you want to open a new one?

FedericoBruzzone commented 1 year ago

To remove useless commit use:

git rebase -i HEAD~5

And type "drop" in place of "pick".

AntonioMiclaus commented 1 year ago

Thanks for all the comments and help. I'm gonna close this request and open a new one in order to avoid further confusion.