-
-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Darkmode #61
Darkmode #61
Conversation
Pull request inspection and corrections
Revert "Pull request inspection and corrections"
Master to fork master
packages/smooth_app/lib/bottom_sheet_views/user_contribution_view.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cool to have a dark mode, thanks!
Some refactoring needed, though.
Don't hesitate to comment if you don't agree.
@M123-dev maybe check if you pulled the lasted version from the Master branch, I changed the smooth_alert_dialog.dart filename to match the naming convention, and also corrected the SmoothAlertDialog class (as @monsieurtanuki noted the context attribute is no longer need). I haven't pull it yet to try but thanks for the PR I will check it out 🙂 |
I find the result quite stunning, well done @M123-dev I'm posting some pictures for those you want to see the result without having to build it : |
@PrimaelQuemerais Thank you for the kind words, my branch is up to date with the master, I don't know why the file has renamed itself again. |
@M123-dev that is weird because it doesn't match the file (filename and code) in the Master branch cf: smooth_alert_dialog.dart |
@monsieurtanuki No problem, I'm also not completely satisfied with the final product myself. Especially the limitations of |
@PrimaelQuemerais yeah I have seen it. It should be fixed now😁 |
Today I revised the PR again. Now all formatting errors should be gone and the navigation bar is now fully supported too. The widgets in the camera function are still normal, in my opinion they are only of little relevance, since most of the screen is covered with the camera image anyways. Of course this should still be added, but this would make more sense after the rebuild of the scanpage #62. (and when scanning works again) When you get on the profile page, there is an error, |
@M123-dev I noticed that the My temporary suggestions, just to locate the issue:
Tell me if it worked. |
@monsieurtanuki That is also what I suspected, however, each change has destroyed any properties of the toggle for me. The first works great, thanks for the tip |
packages/smooth_ui_library/lib/navigation/widgets/smooth_navigation_bar_classic.dart
Outdated
Show resolved
Hide resolved
packages/smooth_ui_library/lib/navigation/widgets/smooth_navigation_bar_classic.dart
Outdated
Show resolved
Hide resolved
@@ -84,9 +91,10 @@ class _SmoothNavigationBarClassicState extends State<SmoothNavigationBarClassic> | |||
.textTheme | |||
.bodyText1 | |||
.copyWith( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that bodyText2
would be much better than bodyText1.copyWith(...)
.
I'm not an expert on themes, but I think the point is to have homogenous colors and text sizes.
Of course for the moment it's a work in progress for all of us, but it would make sense to say: "OK guys, now we use themes, here are the available colors and text sizes, and that's it, no improvisation for each new widget."
What do you think of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When designing the themes, I built in bodyText2
as something like onPrimary
, which should be used as the opposite of bodyText1
in terms of colors. (Such as the icon on the scan button)
bodyText2
currently has no use at all, that means we can use it for all kinds of purposes.
however, I think it is easier to use copyWith in this case, because it isn't a recurring text.
But yes, I agree with you we have to bring better structures into development. Such a kind of contribution guide that lists how to access the translations and how to add something to them, how to access the style elements and code guidelines.
Also, I think we need a better way of project planning, currently there are two roadmaps (README, Google Docs). Also, a big part of the documentation seems outdated.
@PrimaelQuemerais is currently working on a new one, when this is finished I'll look at it and attach a documentation of the data I have brought in and from what I is important.
But since this app is still in its early stage, we probably still have a bit time before we have to think about it seriously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other suggestions you made are now in PR.
packages/smooth_app/lib/main.dart
Outdated
Future<void> main() async { | ||
await Sentry.init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this Sentry part should be part of a separate PR related to #9. This PR is about dark mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought that for the small change it is unnecessary to open a new pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but that's a source of confusion between issues, and there's no obvious sign that you fully tested the Sentry part. Perhaps other contributors that know things about Sentry and nothing about dark mode would like to make suggestions about Sentry, but it will be hidden "in the dark" ;)
That definitely makes sense to create a specific PR for Sentry, and you even already have a specific issue for that (#9).
From my side the pr would be ready now, does anyone still have concerns |
@M123-dev I'll review this PR within a couple of days. After this PR is merged, we'll talk about how we should use providers, shared preferences and themes in this project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@M123-dev As I said, there are things that I would have coded differently, and there are topics we should address in general, like the target use of providers, shared preferences and themes.
But there's nothing I read that would prevent me from approving this PR now.
@monsieurtanuki Thanks again for the feedback and yes as I said, if there are fixed guidelines, I will restructure the code again. |
Like in Issue #22 requested here is a working darkmode, which can be switched on / off on the profile page. The setting is saved via shared prefs.
The darkmode works with the help of provider and does not need a rebuild. A large part of the widgets now support the darkmode function.
The logic is located in the themes folder and there is still only the standard theme in which the variations are specified.
The most important thing that is still missing is the text and the scanbutton and its shadow in the navbar, there I could not follow the code to manipulate the colors.
I am not a graphic designer and this is only the basic principle, everyone is welcome to override my color settings and equip the rest of the widgets with the dark mode.