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

[No QA] Fix various propType warnings #2146

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 30, 2021

Details

Fixed Issues

Fixes No Issue just many propTypes warnings making it hard to debug mobile

Tests / QA Steps

No QA

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron requested a review from a team as a code owner March 30, 2021 00:02
@marcaaron marcaaron self-assigned this Mar 30, 2021
@MelvinBot MelvinBot requested review from TomatoToaster and removed request for a team March 30, 2021 00:02
@Jag96 Jag96 self-requested a review March 30, 2021 17:14
@TomatoToaster
Copy link
Contributor

I think this looks good, just asked @Jag96 to double check me though.

Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Just one question, otherwise LGTM

secondAvatar: {
backgroundColor: themeColors.sidebarHover,
borderColor: themeColors.sidebarHover,
},
}}
} : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, what was the warning here? Having undefined here seems less clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're validating that an Object is being passed, but using a short circuit so the order of operations will literally end up returning false. I guess another (perhaps more clear) way to do this would be

styles={(hovered && !optionIsFocused && styles.someStyle) || {}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also asked Rajat to fix this up in another PR

@Jag96 Jag96 merged commit bbbe320 into master Mar 30, 2021
@Jag96 Jag96 deleted the marcaaron-fixPropTypeWarnings branch March 30, 2021 18:51
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

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