-
Notifications
You must be signed in to change notification settings - Fork 16
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
Navigation Bar Pull Request #112
base: main
Are you sure you want to change the base?
Conversation
const text = universalLayout ? universalText : desktopText | ||
|
||
if (useLogo) { | ||
return ( |
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.
Just a comment for the future
Is there a way we can get these if content into their own components or in a way where this section is not that long, the useLogo logic seems repetitive as well
} | ||
if (Platform.OS === 'ios') { | ||
return { | ||
shadowColor: '#000', |
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.
Maybe this shadow can become a constant
const ANDROID_SHADOW_STYLE
as to reduce the amount of code and not duplicate it?
let variant = | ||
_width > DeviceBreakpoint.TABLET_BREAKPOINT ? 'desktop' : 'mobile' | ||
|
||
let containerStyles = { |
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.
There are lots of styles related code in here, we should look into a cleaner organization.
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.
Also using Stylesheet.create() gives performance benefits,
so non changing styles should be moved to one when possible
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.
You can also use StyleSheet.flatten or Stylesheet.compose
to combine dynamic styles with precreated ones
I would suggest reading a bit around that before moving everything, might be worth the read
src/NavigationBar/index.js
Outdated
<Title variant={variant} titleOptions={title} /> | ||
</View> | ||
<View | ||
style={{ marginLeft: mobileAlignment !== 'right' ? 'auto' : '' }} |
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.
Keeping conditionals outside of jsx allows to separate the presentation and logic layer
usually a
const marginLeft = mobileAlignment !== 'right' ? 'auto' : ''
before the jsx might do the trick
|
||
if (variant === 'desktop') { | ||
// render the title on the left, the menu items in the middle, and the profile image on the right | ||
return ( |
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.
Getting these two jsx into separate components will make the logic/code here more readable
It's me again! Hi hello!
I'm going to keep this open so that y'all can keep track of my code changes and such as I work on the navigation bar project.