Rework comments screen paddings

AppearamidGuy commented 1 year ago

Reworked paddings in comments screen with the same approach as other screens. Also since now recycler has padding equal to status bar, the fix for #61 is not necessary and default scroll behavior will be correct.

Also removed all usages of ViewUtils.getStatusBarHeight which is nice.

SimonHalvdansson commented 1 year ago

This is a nice approach and probably the proper way to treat the problem with the comment navigation buttons. Also getting rid of getStatusBarHeight() is good :)

I tried it out and it worked find on my phone, however it does not work properly on tablets.

When opening a link so that the integrated webview is visible first, the bottom sheet has the wrong top padding - this is fixed once you start to move the bottom sheet.
If the navigation bar height changes (this can happen at any time and calls onApplyWindowInsets) when in the integrated webview mode, the bottom sheet paddings gets messed up again but fixed by scrolling

And one additional thing, when changing the navigation bar height while reading comments, the comments flash for a frame due to recyclerView.setTranslationY being called, perhaps this can be made so this function is not called unless necessary?

AppearamidGuy commented 1 year ago

Thanks, I've missed a minus sign in the insets listener. This should fix both problems

SimonHalvdansson commented 1 year ago

Nice works now, and I did a little thing for the flashing issue also that worked. Thanks for your work! :)

SimonHalvdansson commented 1 year ago

By the way do you think https://github.com/SimonHalvdansson/Harmonic-HN/issues/88 could be related to this? This issue did not use to exist and the result is that when you swipe down from the comments to go the the article you instead scroll to the top and have to swipe again to actually move the sheet which isn't very nice.

AppearamidGuy commented 1 year ago

By the way do you think #88 could be related to this?

Yeah, just rotating the phone is enough for it to end up in a slightly wrong position. It has something to do with the order of loading data into adapter, setting padding and I guess the layout calls.

This issue did not use to exist and the result is that when you swipe down from the comments to go the the article you instead scroll to the top and have to swipe again to actually move the sheet which isn't very nice.

By the way it used to be that you could drag the sheet without letting go of scroll, but this stopped working after some dependency update in 46a4a827c0a051b3b8a1465bf00bec5100c39391

SimonHalvdansson commented 1 year ago

Yeah, just rotating the phone is enough for it to end up in a slightly wrong position. It has something to do with the order of loading data into adapter, setting padding and I guess the layout calls.

I think I can reproduce this reliably now by loading a story which has text which goes longer than the height of the scrool.

By the way it used to be that you could drag the sheet without letting go of scroll, but this stopped working after some dependency update in 46a4a82

Oh yeah I had sort of subconciously noticed this but not really. Either way I think the current behavior is fine and I guess it should prevent some accidental loading of the webpage.

It is not ideal perhaps but I think I fixed the issue by setting the scroll to zero manually before the cache is read in https://github.com/SimonHalvdansson/Harmonic-HN/commit/4c1147b86a801a5dab88cdb51e9910e73d81dff9. I will close the issue for now with this fix because I couldn't find any side effects of this.

Edit: I mean I'll close the other issue I linked earlier.

SimonHalvdansson / Harmonic-HN

Rework comments screen paddings #71