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

Refactor(MediaCheckDialog): Media check to use ViewModel #17943

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

criticalAY
Copy link
Contributor

@criticalAY criticalAY commented Feb 8, 2025

Purpose / Description

Migrates the logic of MediaCheck to ViewModel
Split the dialog logic, use different dialog for result and conformation

Approach

Use ViewModel to handle the media check dialog and its result

How Has This Been Tested?

Tested on Google emulator API31
Screen_recording_20250209_030932.webm

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Contributor

github-actions bot commented Feb 8, 2025

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@criticalAY criticalAY changed the title Ref(MediaCheckDialog): Media check to use ViewModel Refactor(MediaCheckDialog): Media check to use ViewModel Feb 8, 2025
@criticalAY criticalAY added the Blocked by dependency Currently blocked by some other dependent / related change label Feb 8, 2025
Comment on lines 1960 to 1962
withCol {
tags.bulkAdd(missingMediaNotes, TR.mediaCheckMissingMediaTag())
}
Copy link
Member

Choose a reason for hiding this comment

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

These 3 lines should be in the ViewModel: returning a Job which we .join() on.

The ViewModel should contain a TODO that we want to move progress notifications here.

(See #17804 for how this could/should be done)

- use ViewModel to handle the result in the dialog
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch from 18ef3d5 to 6b62901 Compare February 10, 2025 13:52
@criticalAY criticalAY added Needs Review and removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Feb 10, 2025
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.

2 participants