-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
fix(SearchBar): apply light and dark theme when platform ios or android #3470
base: next
Are you sure you want to change the base?
Conversation
…bar design updates
Codecov Report
@@ Coverage Diff @@
## next #3470 +/- ##
==========================================
- Coverage 79.24% 79.22% -0.03%
==========================================
Files 87 87
Lines 1812 1810 -2
Branches 813 811 -2
==========================================
- Hits 1436 1434 -2
Misses 370 370
Partials 6 6
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@@ -18,21 +18,21 @@ export type { SearchBarAndroidProps }; | |||
const defaultSearchIcon = (theme: Theme) => ({ | |||
type: 'material', | |||
size: 25, | |||
color: theme?.colors?.platform?.android?.grey, |
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.
can do same for iOS
height: 1, | ||
}, | ||
shadowOpacity: 0.2, | ||
shadowRadius: 1.41, |
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.
don't think elevation/shadow is needed
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 was trying to design like this https://material.io/design/navigation/search.html#persistent-search
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.
Let's not hard-code it If user need elevation they can just pass it through style prop?
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.
ok I will remove that
@@ -50,6 +50,7 @@ export class SearchBarAndroid extends Component< | |||
input!: TextInput; | |||
|
|||
static defaultProps = { | |||
lightTheme: false, |
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.
lets remove lightTheme
from each variant & keep background as theme.colors.searchBg as you did
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 have checked that if we remove the lightTheme then SearchBar will be dark
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.
Ohh, try changing searchbg for dark color
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.
please check this e52099b commit, can we use separate colors for background Color as per platform?
…native-elements into SearchBar_updates
…native-elements into SearchBar_updates
package.json
Outdated
@@ -67,8 +67,7 @@ | |||
"lint-staged": { | |||
"packages/**/*.{ts,tsx}": [ | |||
"eslint --fix", | |||
"prettier --write", | |||
"tsc --noEmit --composite false" |
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.
Can you undo this change, use --no-verify
flag in case you feel it takes lot of time
SearchBar design updates
Motivation
currently, we can use both light and dark themes only when the platform is default and when the platform is android or ios then we can use only light theme
Fixes # (issue)
#3466
Type of change
How Has This Been Tested?
example
appChecklist
Additional context