-
Notifications
You must be signed in to change notification settings - Fork 134
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
Add a configuration to allow the display of the Toast above the mini-player #2555
Open
SergioEstevao
wants to merge
4
commits into
trunk
Choose a base branch
from
sergio/show_toast_above_mini_player
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
55b82ef
Add a configuration to allow the display of the Toast above the mini-…
SergioEstevao 8a5ffe0
Allow configuration of the inset provider when displaying the toast
SergioEstevao 4ad4b58
Merge branch 'trunk' into sergio/show_toast_above_mini_player
SergioEstevao 85ba7b3
Take in account the offset to determine the size of Toast
SergioEstevao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
We'll probably forget this if we change the mini-player and the toast will probably appear incorrectly again. Also, given this is a property I see one of us repeating the same issue
I wonder if there's any way we can:
However, such a solution still couple this to the mini-player and might require some additional hack code. 🤷
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.
The modifier actual ensures that the miniPlayer is taking in account the issue we have is the tabbar, while on the other places the standard safeInset ensure thats the bottom already takes in account the tab bar, on the toast because we are on another Window the tab bar is not taken in account.
Do you see a way to detect the tab bar presence and size from another window?
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.
@leandroalonso @SergioEstevao this solution it's similar to what Android team has implemented. In our codebase we have other classes, like the
InsetAdjuster
, that takes the same argument to adjust the padding.However, if we want to automatically adjust it without injecting any parameters, we can use the
NavigationManager
(with some adjustments) as source to get the miniplayer and tabbar access.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 think it should be possible to adjust using the
MainTabBarController
at theToastWindow
level but I wasn't quite able to get the values right:I was hoping the
keyWindow.safeAreaInset
would include the tab bar height but it does not seem to right now and adjusting that would probably mess with a bunch of things.Would this make sense as an alternative? It's still somewhat messy to pull out that
MainTabBarController
and make sure there's nothing presented.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.
@bjtitus The
NavigationManager
singleton has this information.AppDelegate
and theMainTabBarController
rely on this. If we want to have an auto position of the Toast I think we can use this as source.Otherwise my suggestion is to use Sergio solution and align with Android
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.
@danielebogo and @leandroalonso I updated the code so that instead of hard-coding the miniplayer inset attribute on toast, I added a inset provider parameter that allows the customization of the inset depending of the screen.
By default it will use the safeAreaInset.bottom but customizations can be made to for example take in account the mini-player.
Give it a look and tell me what you think.
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.
@bjtitus ^