Skip to content
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

Theme change functionality #22

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AliAkberAakash
Copy link
Collaborator

Why? (ticket link or issue description)

#16

What was done and how?

Theme changing was implemented with GetX

Anything special? (optional)

Copy link
Owner

@hasancse91 hasancse91 left a comment

Choose a reason for hiding this comment

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

We need more flexibility to manage every widget color of the app. Cannot rely on GetX's basic theme changing. Please update your PR to achieve more customization option. For example below screenshots. item card's background should be dark color and text should be light color.

Screenshot_20211220-112000
Screenshot_20211220-112302

import 'app_colors.dart';

class AppThemes {
// light theme configurations
Copy link
Owner

Choose a reason for hiding this comment

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

Our code should be self explanatory. don't need to add unnecessary comment.

);
}

// dark theme configurations
Copy link
Owner

Choose a reason for hiding this comment

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

Our code should be self explanatory. don't need to add unnecessary comment.

Comment on lines 13 to 30
// Get themeMode info from local storage and return current ThemeMode
Future<ThemeMode> get themeMode async =>
await _loadThemeFromSharedPref() ? ThemeMode.dark : ThemeMode.light;

// Load themeMode from local storage
Future<bool> _loadThemeFromSharedPref() async {
return await _preferenceManager.getBool(themeModeKey);
}

// Load isDarkMode status from local storage
Future<bool> getCurrentThemeMode() async {
return await _preferenceManager.getBool(themeModeKey);
}

// change isDarkMode status in local storage
changeThemeMode(bool isDarkMode) {
_preferenceManager.setBool(themeModeKey, isDarkMode);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Our code should be self explanatory. don't need to add unnecessary comment.

Comment on lines 13 to 19
getCurrentThemeMode() async {
currentThemeMode.value = await themeService.themeMode;
}

setCurrentThemeMode(ThemeMode themeMode) {
currentThemeMode.value = themeMode;
}
Copy link
Owner

Choose a reason for hiding this comment

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

add return type as void.

@AliAkberAakash
Copy link
Collaborator Author

We need more flexibility to manage every widget color of the app. Cannot rely on GetX's basic theme changing. Please update your PR to achieve more customization option. For example below screenshots. item card's background should be dark color and text should be light color.

Screenshot_20211220-112000 Screenshot_20211220-112302

Here the background color of the cards are static colors from AppColors file, to make it dynamic we need to use colors from the theme regardless of we use GetX theme or our custom theme class. Should I refactor them to use colors from theme?

@hasancse91 hasancse91 marked this pull request as draft December 27, 2021 06:53
@hasancse91 hasancse91 linked an issue Jan 13, 2022 that may be closed by this pull request
@AliAkberAakash AliAkberAakash marked this pull request as ready for review January 15, 2022 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Light and dark theme switching option should be in Settings page
4 participants