This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
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.
Room call banner #9378
Room call banner #9378
Changes from 14 commits
194cdec
29f48ed
afaee18
af4417f
13349c1
3f3f35e
fa5ccde
b2f53cb
3a81bfe
8b7bd6a
17c6db3
fecd428
11ed620
f792d01
211c9bc
8e8d02d
01b708e
7a94db3
bebe0ff
8df7762
6d84327
9741353
55146e7
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.
If the comment could mention why, it could be helpful - I'm assuming because they just occupy the same space, in which case this i obviously a bit nonideal & we probably want something a bit more like the toast manager that decides which one takes precedence (but no need to block on that part).
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.
yes, carry also proposed a better solution than what it currently is. (this makes testing easier though (I think) since it is all inside one component)
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.
Could avoid having RoomCallBanner depend on beacon store by doing something like:
Would keep things simpler in RoomCallBanner, and make this more future proof
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.
That makes sense. It would make the tests involve the whole Room Header though.
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.
Do you think it is fine leaving it like this in favor of not changing the tests?