Fix Layout issues in EmojiDialog for several edge cases. #326
Conversation
|
@vanniktech well, I can try :) Would appreciate testing in the mean time! |
|
It behaves very weirdly on my emulator.
Also toggling with the Emoji button no longer seems to work. |
|
Can you let me know which emulator is that so I can take a look? (what Android version) |
|
|
|
Yea, I just did reformatting to comply with your rules. The code did work, so maybe I didn't push the right branch - I'm investigating. |
|
Should be ready now. I've experienced some weird "dismiss" animation issues but those seem to be there before as well. This fixed extracted UI issues + issues with phones like P20 pro. |
|
As @vanniktech said, this is really hard to review. I know it is not fun, but can you revert all style changes, not related to your actual changes (And preferably use a style fitting the rest of the repo)? |
|
@rubengees I now use the exact same style as you folks do (Square Android) and the formatting has been done according to those guidelines. Can you clarify what should I change? Do you mean the private modifiers I've added and such? |
|
No, those are fine! I'm very sorry for being so picky, but these changes add noise to the PR, making it hard to focus on the actual changes. I mean things like below. |
|
So hard on the new contributors :P Anyway, I tried ... let me know if I missed something, it's getting quite late :) |
There was a problem hiding this comment.
Looks good and it does work with the emulator. Do you think this will fix #191?
And yeah style guidelines and everything is a bit stricter here. 🤷♂️
There's still one glitch though (the bottom part isn't filled correctly)
|
@vanniktech can you let me know where (what phone, what OS version) do you experience the glitch with the bottom part not filled? As for #191, I believe so - I'll take a look at my S7 - might not be the EXACT same phone, but oh well. |
|
(I can also take care of the formatting in case it's too exhausting for you.) We'll get there slowly. Emulator is Nexus 4 with API 28. And on my Pixel 3 this also fails with the same behavior which is on Android P too. |
|
@vanniktech I plan on contributing in the long term unless something serious happens, so I'd rather slowly familiarize myself with the guidelines and all. Luckily, I've been in FOSS for my whole life so I don't find the approach off-putting (and understand the reasoning), but I can understand how a new contributor might want to get away after so many style requests on their first PR :P |
|
Formatting is changed, but unfortunately I can't reproduce the layout issue you have :( Nexus 4, API 28 as follows:
Are there any "special" steps I could take to try and reproduce the issue? |
Signed-off-by: Mario Danic <mario@lovelyhq.com>
|
Weird. Not that I know. I do think that the notch isn't supported which is why it's failing on my Pixel 3XL. |
|
@vanniktech my P20 Pro has a notch, and it works as expected. Can you try to clean & rebuild? Probably not it, but you never know. |
|
Nah didn't solve it. |
|
Ah missed the two, sorry about that. Will try on a few more devices, but it's really strange that your Nexus 4 emulator (which is the same as mine!) behaves differently. |
|
I just simulated a cutout too (both regular & double) and it works. Uh. |
|
Ah found a way to reproduce. Will fix. |
Signed-off-by: Mario Danic <mario@lovelyhq.com>
|
Can you folks try it now please? |
vanniktech
changed the title
|
Thank you very much! |
|
Thanks fo both of you for testing and doing code review. |
|
Thanks for this change @mario! I was not in time to review, but I integrated this commit in one of my apps and confirmed with a user having a similar problem, that it now works! |
|
If you want @rubengees next time I'll wait for your review too. |
This PR fixes a few layout issues I've noticed, more importantly:
This is still WIP as I want to clean it up and test, but feel free to test it yourself and comment - all feedback is more than welcome.
Signed-off-by: Mario Danic mario@lovelyhq.com