search

amugofjava / anytime_podcast_player

Simple, easy to use Podcast player app written in Flutter and Dart.
BSD 3-Clause "New" or "Revised" License
376 stars 99 forks source link

Fix jumping when collapsing expanded description #123

Open filip-alexandrov opened 2 months ago

filip-alexandrov commented 2 months ago

Attempts to fix the closing animation of #84. I wanted to get your input on the "Show more" button position, since for me the more natural position is just below the text. If you don't aggree I will revert the changes to the original button implementation.

Implementation

I have removed the isDescriptionExpandedStream in the PodcastTitle component, since I moved all expansion logic in the stateful PodcastDescription component. Toggle the description container by:

The widget first computes the HTML content size (height) inside 0px high ScrollView (so the content retains its original size). Then it expands the AnimatedContainer to either _maxCollapsedHeight (which is 100px) or if the content is less than 100px, it expands to this smaller size and hides the “Show more” button. If the user clicks on the button or the description content itself, the container expands to its full height.

Please let me know if I can improve this contribution. Thanks for the cool project 👍

https://github.com/amugofjava/anytime_podcast_player/assets/79532720/151f4ee9-5679-4300-a5b9-3b7bbdd6f407

amugofjava commented 2 months ago

Hi @filip-alexandrov,

Thank you for this PR; this is great. The animation is much better and I like the 'show more/show less' text - and, combined with a suggestion below will help improve accessibility. I do have some feedback:

Could you disable the animation during initial podcast load. When you go into a podcast the animation is triggered and the description slides into place which looks a little off.

Screen_recording_20240414_104116.webm

Please can you wrap the text and show more/show less button in a MergeSemantics widget, so that a screen reader will see this as a single widget. It won't affect it working, but will reduce the amount of swipes required to navigate.

The Show more and Show less text will need translating into German and Italian. For short phrases I find a combination of Google Translate and Bing Translator very useful. There is a TRANSLATION.md document to help with the process if you're happy to do this. Please let me know if you need some help with this.

Reviewing your PR has highlighted an existing bug with the top margin & padding of the text block varying between podcasts. I hadn't spotted this before, but it's more obvious with the Show more/Show less text in place. I don't know if this is an issue you would be interested in taking a look at and incorporating it into this change. #124

Thanks again.

filip-alexandrov commented 2 months ago

Hi @amugofjava, thanks for the feedback. I've proceeded to add the MergeSemantics widget, german and italian translations. Hopefully I've done everything correctly :)

Also the margin issue (#124) is in the last commit, it's just a single line. Please let me know if there are any issues with the code 👍

amugofjava commented 2 months ago

Hi @filip-alexandrov, I've made a couple of comments on the code.

I have also made a few changes in master as I noticed that I wasn't rendering the follow, menu widgets etc upon first render which could result in them jumping in to view which looked a bit odd.

amugofjava commented 2 weeks ago

Hi @filip-alexandrov,

Just checking in to ask if all is well and if there is anything I can do to help you finish off this PR?

Thanks,

Ben.