Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(in-app-messaging): add BannerMessage UI component #9124
feat(in-app-messaging): add BannerMessage UI component #9124
Changes from 1 commit
436e683
01128d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: This is not blocking but I wonder if
styles
should bedefaultStyle
or something? I keep finding myself having to remind myself what the difference betweenstyles
andstyle
is. As a reader of foreign code, I would have no idea why they're different things.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.
imo default styling as
styles
is a pretty standard convention in React Native. That said i do think all the styling applied in these components is not the easiest to parse 😟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.
This is a good point too since static StyleSheets are typically assigned to a
styles
variable. It was just a thoughtThere 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.
What happen if there is only
secondaryButton
?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.
Ideally, the contract we have with pinpoint should prevent that from happening, but it seems reasonable to add handling for that scenario
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.
Nit: You could extract this to
../constants
as well so that it doesn't need to be recalculated if it needs to be used elsewhere.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 like the grouping approach you did here :)
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.
Nit: I'm not yet familiar with the React Native codebase's patterns, but in the future, it could be good to prefix the color
const
s withCOLOR_
- i.e.COLOR_BLACK
,COLOR_WHITE
,COLOR_GREY_LIGHT
(similar toBANNER_
) in order to group the declarations closer together.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.
Nit: Consider switching this around - i.e.
SPACING_X
so developers can easily access spacing values rather than typingLARGE
and then having to choose spacing vs fontsize etc. This could even be aSPACING
object that contains different size properties.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.
Agree with naming, but prefer to keep these explicit exports. Basically feel like putting them in an object implies they should be long lived instead of temporary
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.
Why
MAX_MESSAGE
instead ofMAX_MESSAGE_LENGTH
?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.
No real reason, they are just for my testing