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

Feature: Dark Mode Support #1 #243

Merged
merged 5 commits into from
Feb 2, 2024
Merged

Feature: Dark Mode Support #1 #243

merged 5 commits into from
Feb 2, 2024

Conversation

juanchoperezj
Copy link
Contributor

@juanchoperezj juanchoperezj commented Aug 25, 2023

Pull request type

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Description

Introducing Dark Mode Support using React Navigation Theme.
Note: This PR was started from the branch feature/i18n to take advantage of the Settings screen.


Jira board reference


Notes:

  • A few changes were made around the Settings Screen UI
  • New translations added

Tasks:

  • Tested on IOS
  • Tested on Android
  • Tested on a small device
  • Tested on a real device
  • Tested all flows related with this PR changes
  • Tested accessibility
  • Added tests

Preview

UI Changes will be on the second part of this feature.

@juanchoperezj juanchoperezj changed the base branch from main to feature/i18n August 25, 2023 19:11
@juanchoperezj juanchoperezj self-assigned this Aug 25, 2023
@juanchoperezj juanchoperezj changed the title Feature: Dark Mode Support Feature: Dark Mode Support #1 Aug 25, 2023

return (
<SafeAreaView style={styles.container}>
<Text accessibilityRole={'text'} style={styles.title}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Text accessibilityRole={'text'} style={styles.title}>
<Text accessibilityRole="text" style={styles.title}>

</Text>
<Button
accessibilityState={{ disabled: false }}
title={'⚙️'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title={'⚙️'}
title="⚙️"

jpirazusta
jpirazusta previously approved these changes Aug 28, 2023
Copy link
Contributor

@jpirazusta jpirazusta left a comment

Choose a reason for hiding this comment

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

👏

ManuelJrez
ManuelJrez previously approved these changes Aug 31, 2023
Copy link
Contributor

@ManuelJrez ManuelJrez left a comment

Choose a reason for hiding this comment

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

Maybe another improvement would be create a useStyles or useThemedStyles hook that we could use to generate styles for any component just to avoid creating a function called useStyles that uses the useTheme to get access to the theme but WDYT?? @juanchoperezj

Nice work here!! 🚀

thumbColor={WHITE}
ios_backgroundColor={WHITE}
onValueChange={toggleTheme}
value={currentTheme === ColorScheme.dark}
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT if we could add isDarkMode to the useThemeConfig just to get that constant in all places 🤔

Suggested change
value={currentTheme === ColorScheme.dark}
value={isDarkMode}

Base automatically changed from feature/i18n to main August 31, 2023 21:46
@juanchoperezj juanchoperezj dismissed stale reviews from ManuelJrez and jpirazusta August 31, 2023 21:46

The base branch was changed.

jpirazusta
jpirazusta previously approved these changes Sep 4, 2023
ManuelJrez
ManuelJrez previously approved these changes Sep 12, 2023
@juanchoperezj
Copy link
Contributor Author

Maybe another improvement would be create a useStyles or useThemedStyles hook that we could use to generate styles for any component just to avoid creating a function called useStyles that uses the useTheme to get access to the theme but WDYT?? @juanchoperezj

Nice work here!! 🚀

I'll add this hook from the branch #244 but in a new one to narrow the number of files changed.

Copy link
Collaborator

@fernandatoledo fernandatoledo left a comment

Choose a reason for hiding this comment

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

Great work! Only left a few comments :)

return (
<SafeAreaView style={styles.container}>
<Text accessibilityRole="text" style={styles.title}>
Home Screen
Copy link
Collaborator

Choose a reason for hiding this comment

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

This string could be moved to the locale :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment on lines 41 to 46
<TouchableOpacity
disabled={language === 'es'}
onPress={() => onChangeLanguage('es')}
style={[styles.button, language === 'es' && styles.disabledButton]}>
<Text style={styles.buttonText}>Spanish</Text>
</TouchableOpacity>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not making use of the button component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! I was using the following because of the styles, I didn't want to add other props to the button component because is not related to the PR changes.

<View style={styles.option}>
<Text style={styles.optionTitle}>{translate('screen.settings.changeLanguage')}</Text>
<Button
title="Spanish"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need to add language texts to the locale files, those could be handled differently according to the project's needs.

@juanchoperezj juanchoperezj requested review from Stukz, ManuelJrez and asdolo and removed request for ManuelJrez December 12, 2023 22:45
@juanchoperezj juanchoperezj merged commit 42ca274 into main Feb 2, 2024
4 checks passed
@juanchoperezj juanchoperezj deleted the feature/darkmode branch February 2, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants