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

[FEATURE REQUEST] Add status message when (un)setting av. offline from preview #4482

Merged
merged 15 commits into from
Oct 10, 2024

Conversation

joragua
Copy link
Collaborator

@joragua joragua commented Oct 1, 2024

Related Issues

App: #4382

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

Checks done: #4482 (comment)

Reports:

@CLAassistant
Copy link

CLAassistant commented Oct 1, 2024

CLA assistant check
All committers have signed the CLA.

@joragua joragua linked an issue Oct 1, 2024 that may be closed by this pull request
16 tasks
@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch 2 times, most recently from 554ca38 to e892103 Compare October 1, 2024 11:08
@joragua joragua self-assigned this Oct 2, 2024
@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch 3 times, most recently from ac8ab37 to 3deecbd Compare October 2, 2024 12:35
@joragua joragua changed the title [FEATURE] Add status message when (un)setting av. offline from preview [FEATURE REQUEST] Add status message when (un)setting av. offline from preview Oct 2, 2024
@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch from 091c1a4 to 8eaa4e9 Compare October 3, 2024 12:03
@joragua joragua requested a review from JuancaG05 October 3, 2024 12:26
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Good job @joragua! Some comments and questions here 👍

@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch from 2e420fc to ca5c106 Compare October 8, 2024 06:23
@joragua joragua requested a review from JuancaG05 October 8, 2024 06:24
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some more comments and changes here @joragua 🧑🏻‍💻

@JuancaG05
Copy link
Collaborator

There are several KtLint reports as well in the BitRise workflow, take a look at them and fix them. You can rebase this branch against master as well @joragua 👍

@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch 2 times, most recently from 3a61024 to b93b787 Compare October 8, 2024 09:19
@joragua joragua requested a review from JuancaG05 October 8, 2024 09:58
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

One more tiny change and we're done @joragua!

@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch from cdbc870 to 22aed52 Compare October 9, 2024 07:26
@joragua joragua requested a review from JuancaG05 October 9, 2024 07:35
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM now! Good job @joragua 🏅

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 9, 2024

Checks done:

  1. Set as av. offline (both orientations)
  • Image preview
  • Text preview
  • Audio preview
  • Video preview
  1. Unset as av. offline (both orientations)
  • Image preview
  • Text preview
  • Audio preview
  • Video preview

Image, text and audio are always downloaded whwn previewed. Videos can be streamed, but during the streaming, the av. offline options are not available, that means, no extra case there.

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 9, 2024

(1) [FIXED]

just a tiny detail i noticed: in the image previews, the snackbar is smaller (in height) than in other previews. I guess that is something related with system, but it took my attention. I tested with two different devices. Check the difference with the following short videos:

txt preview image preview
https://github.com/user-attachments/assets/f25a79da-783d-4ad1-9fbb-d580cd838934 https://github.com/user-attachments/assets/9407ce38-ce91-49fc-bb43-1e82cb993bdf

i suggest to download both of them and play together to compare.

Pixel 2, Android 11
Xiaomi Redmi 13, Android 14
cdbc8705

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 9, 2024

Related with (1), by setting as av. offline in landscape orientation (image preview):

Screen_recording_20241009_110643.mp4
  • The snackbar does not rise from the bottom (maybe because the margin)
  • The android bar, placed to the left, overlaps part of the message

could this be part of the same issue?

@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch from d7545ab to fa39250 Compare October 9, 2024 12:30
@joragua
Copy link
Collaborator Author

joragua commented Oct 9, 2024

(1) I think it's fixed. requireActivity().window.decorView was the solution, because the activity manages the view of image previews and not the fragment.

Take a look if you can 😀

@joragua joragua requested a review from JuancaG05 October 9, 2024 12:40
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 10, 2024

(1) fixed

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 10, 2024

Approved on my side.

There are some conflicts in the branch because the changelog items, could you take a look before merging @joragua ?

@joragua joragua force-pushed the feature/add_feedback_in_preview_menu branch 2 times, most recently from 2c40fcc to 97dbbbf Compare October 10, 2024 08:56
@joragua joragua merged commit 51c910a into master Oct 10, 2024
3 checks passed
@joragua joragua deleted the feature/add_feedback_in_preview_menu branch October 10, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Add status message when (un)setting av. offline from preview
4 participants