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

Fixes some strange layout issues in EpisodeFragment #459

Merged
merged 5 commits into from
Oct 31, 2022

Conversation

VGJohn
Copy link
Contributor

@VGJohn VGJohn commented Oct 22, 2022

Fix for some strange but pretty serious layout issues in the EpisodeFragment seen in the linked issues.

All the issues are stemming from the WebView with height=WRAP_CONTENT that displays the episode notes. The height of the WebView causes different issues on different episodes, if the notes in the WebView fit on screen then the WebView flickers and disappears and then if the title of the episode fits on a single line then the entire screen is blank.

I've attached videos showing the before and after with fix. I'm testing on a Pixel 6a with my language set to German with default font size setting. I'm also forcing the notes to just be "Testing" and the title of every episode to just be "Testing" which causes the strange layout issues seen in the video.

NOTE: There is one drawback to this fix, if the episode notes are short then they display centred at the end of the screen and same thing with the progress spinner that shows while the notes are loading. This can be seen in the after video. I think this is an acceptable trade off considering users would see no notes or a blank screen before.

Fixes #162 #163

Checklist

  • Should this change be included in the release notes? If yes, please add a line in CHANGELOG.md
  • Have you tested in landscape?
  • Have you tested accessibility with TalkBack?
  • Have you tested in different themes?
  • Does the change work with a large display font?
  • Are all the strings localized?
  • Could you have written any new tests?
  • Did you include Compose previews with any components?

@VGJohn VGJohn requested a review from a team as a code owner October 22, 2022 20:01
@VGJohn
Copy link
Contributor Author

VGJohn commented Oct 22, 2022

Before video showing the issues on various episodes

before.mp4

@VGJohn
Copy link
Contributor Author

VGJohn commented Oct 22, 2022

After video showing the fix on the same episodes (can also see the effect the fix has on the loading spinner and placement of short episode notes, again I feel like this is an acceptable tradeoff)

after.mp4

@ashiagr
Copy link
Contributor

ashiagr commented Oct 28, 2022

Thank you for the fix @VGJohn and sorry about the delay in reviewing it. While your solution looks fine but as you stated, it comes with a drawback that'll impact episodes with shorter notes.

I wonder if the issue could be because of how webViewShowNotes is constrained w.r.t. a group element lblDate.
lblDate is a loadingGroup element aligned to bottomDivider which depends on another group: errorLayout. Both loadingGroup and errorLayout group visibility is controlled dynamically.

There's a known Android issue that affects the visibility of group elements and I suspect it could affect elements constrained to Group or their elements.

I wasn't able to reproduce the issue (tested on Pixel 5, Pixel 3a devices, and Pixel 6 emulator
after force changing notes and title to Testing on the main branch) so I could not verify. By any chance, do you have steps to reproduce the issue on an emulator?

@VGJohn
Copy link
Contributor Author

VGJohn commented Oct 30, 2022

@ashiagr sorry about the delay as well!

Thanks for the tip about the Group, I tried removing them although that didn't change the behaviour I was seeing but it did make me suspicious of the Barrier. The issue was arising from the middle part of the layout with all the buttons, instead of using the Barrier I switched to just wrapping the buttons in a nested ConstraintLayout.

The result is that the issues from the before video are fixed and the layout remains unchanged 🎉

I tried to find another device or emulator that I could reproduce the issue on but it seems like it only happens on the Pixel 6a which is very strange, sorry about that!

device-2022-10-30-033453.mp4

@VGJohn
Copy link
Contributor Author

VGJohn commented Oct 30, 2022

Attaching a video of the episode screen with the actual episode title and notes instead of them being hardcoded to "Testing" so you can see the layout remains the same with this fix 😄

device-2022-10-30-033923.mp4

Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

This is amazing @VGJohn! 🥳

I could not reproduce the issue on any of my devices or emulators but your changes look good to me.

Thank you so much for your continued effort in fixing it. 🙏

@VGJohn
Copy link
Contributor Author

VGJohn commented Oct 31, 2022

Cheers @ashiagr

I think I just got lucky for the most part haha but hopefully it fixes the issues for the german users that reported it ❤️

Let me know if there's anything I can do to fix the spotless check failing, I run the spotless gradle task before opening any PRs and I didn't see it complain about any of the changes I made but I can't view the failure details from buildkite so maybe its something else 😕

@ashiagr
Copy link
Contributor

ashiagr commented Oct 31, 2022

Spotless is working fine on the local copy. It looks like some caching issue, I re-triggered the build to see if it fixes it.

I don't think you need to do anything, but I'll keep you informed. 🤞

@ashiagr ashiagr merged commit 5dc414b into Automattic:main Oct 31, 2022
@ashiagr ashiagr added this to the 7.27 milestone Oct 31, 2022
@ashiagr
Copy link
Contributor

ashiagr commented Oct 31, 2022

CI got working this time, it was a temporary glitch.

hopefully it fixes the issues for the german users that reported it

I hope so too 🤞

I merged your PR, only to realize that there was no ChangeLog entry to this fix. Could you add it in your other open PR or a new one if you like?

@VGJohn
Copy link
Contributor Author

VGJohn commented Oct 31, 2022

@ashiagr sorry about that, I added the changelog entry to my other open PR 😄

@VGJohn VGJohn deleted the episode-description-fix branch October 31, 2022 20:11
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.

Episode description display issue on localized screen
2 participants