Quick Editor - Alt text feature foundations #501

hamorillo · 2024-12-17T10:21:45Z

Closes #497

Description

This PR implements the foundations of the Alt Text feature. It doesn't include all of the functionality, as some parts still need to be implemented/improved. See sub-issues

I'm opening this PR for two reasons:

I think we can parallelize the missing work
To keep this PR as small as possible

The basic functionality, which is changing the alternative text for an image, is implemented.
You should be able to see the confirmation toast and the error toast when something fails.

Testing Steps

Open the QE and the options for any avatar
Modify the alt text
Verify the Save is enabled when the value is different from the original alt text
Save a new alt text
Verify the QE navigates to the Avatar picker
Close the QE bottom sheet and reopen it
Open the alt text for the previous image and verify you see the new alt text
Try to change it again but force an error (network error is enough)
Verify you see the error toast
Open the alt text again and verify you see the previous alt text (not the one that produces the error)
Verify dark/light mode
Verify orientation changes

wpmobilebot · 2024-12-17T10:24:32Z

📲 You can test the changes from this Pull Request in Gravatar Demo by scanning the QR code below to install the corresponding build.

	App Name	Gravatar Demo
	Commit	`29c3a14`
	Direct Download	`gravatar-demo-prototype-build-pr501-29c3a14.apk`

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/components/AltTextSection.kt

AdamGrzybkowski

I've left some comments/ideas that I think are worth discussing. Additionally, I think we need to handle the system back button to hide the Alt text section only, not the whole bottom sheet. There's no way to exit this state now without saving the new Alt text.

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/components/AltTextSection.kt

AdamGrzybkowski · 2024-12-17T12:59:25Z

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/avatarpicker/AvatarPicker.kt

+            if (uiState.mainSection is MainContentSection.AvatarsSectionUiState) {
+                key(uiState.avatarUpdates) {
+                    ProfileCard(
+                        profile = uiState.profile,
+                        modifier = Modifier.padding(horizontal = 16.dp),
+                    )
+                }


❓ Have you considered creating a new page for the Alt text functionality? We could use nested graphs for this. Although it might look more time-consuming, I think it could lead to:

Simplified UI code. There wouldn't be a need for several checks like this to show/hide section.

Potentially simpler integration with the Close/Cancel button from the TopBar. This is a very top-level component, so having a nested destination would simplify working with it as it's closer.

If you prefer to keep it like this, maybe we should wrap the whole AvatarPicker (card and the section) and only do one of those checks (if section) in one place. This would also simplify any type of enter/exit animations I think.

I'll take a look at the nested graphs. Thanks for the suggestion.

I've done a lot of refactoring in this PR using a nested graph. As you mentioned, the code looks cleaner (at least in my opinion).

Potentially simpler integration with the Close/Cancel button from the TopBar.

This still will cause some "problems" so we'll need to figure out how to solve this. One option could be to move the TopBar inside each section, but I haven't thought deeply about this.

Anyway, let me know what you think about the current approach.

I think we can still do another step and create a different ViewModel for the AltText section. Extracting the code related to the AltText should be "easy", the problem here is that we'll need to also make a way to sync the data between them. We can iterate over this in the following PRs.

I think we can still do another step and create a different ViewModel for the AltText section. Extracting the code related to the AltText should be "easy", the problem here is that we'll need to also make a way to sync the data between them. We can iterate over this in the following PRs.

I think we should do that. I left a comment related to that.

Working on it ⚙️

I've extracted the AltText related code to it's own ViewModel. The AvatarPickerViewModel handles only the rating changes so I've also refactored that code to remove all the unnecessary stuff.

we'll need to also make a way to sync the data between them

This PR doesn't accomplish that part. I think we should work on that in a different PR. For the moment, the AltText won't be updated in the local data models (it's updated in the backend) until the bottomsheet is closed and reopened again. (Which forces the data to be loaded from the server). #516

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/components/AltTextSection.kt

AdamGrzybkowski · 2024-12-17T13:11:48Z

One more and this could be left as a subtask for further improvements, but I think it would be great to set the max height of the input field and make the content of it scrollable in a way the the Save button would remain visible even with a longer text.

hamorillo · 2024-12-19T18:03:37Z

I think it would be great to set the max height of the input field and make the content of it scrollable in a way the the Save button would remain visible even with a longer text.

We have an issue to limit the number of characters #502, when we implement that feature, I would say this won't be a huge problem, but we'll see.

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextSection.kt

AdamGrzybkowski

That looks better now I think! I've left a few comments that are worth addressing first.

I noticed that the Avatar in the profile card refreshes after coming back from the AltText section. I believe it's not directly related to this PR, but something that we should take a look at.

...atar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/editor/GravatarQuickEditorPage.kt

AdamGrzybkowski · 2024-12-20T09:13:00Z

...atar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/editor/GravatarQuickEditorPage.kt

+    val editorViewModel: AvatarPickerViewModel = viewModel(
+        factory = AvatarPickerViewModelFactory(
+            gravatarQuickEditorParams = gravatarQuickEditorParams,
+            handleExpiredSession = true,
+        ),
+    )


⚠️ There's a slight change in behavior here. This will make the AvatarPickerViewModel service QE relaunches. You can see that when you close/open the QE, there's no initial loading.

I wonder if this would be problematic for apps that provide the token. In a scenario where the user changes but the ViewModel survives and persists the previous data.

My suggestion would be to opt for a separate ViewModel anyway. So maybe it's not worth thinking about how to fix that.

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextSection.kt

AdamGrzybkowski · 2024-12-20T09:39:33Z

I created an issue for the Avatar reload - #514

In the Figma file the height is fixed to 50.dp

…AltText

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextPage.kt

hamorillo · 2024-12-30T14:49:05Z

...-quickeditor/src/main/java/com/gravatar/quickeditor/ui/avatarpicker/AvatarPickerViewModel.kt

        }
    }

-    private fun updateAvatar(avatarId: String, rating: Avatar.Rating? = null, altText: String? = null) {
+    private fun updateAvatarRating(avatarId: String, rating: Avatar.Rating? = null) {


The AvatarPicker modifies only the Rating so there is no need for the generic method that allow the altText update. I've remove/renamed all the related code.

AdamGrzybkowski

Thanks for all the updates! Tested it and all worked fine!

AdamGrzybkowski · 2025-01-07T08:55:04Z

...atar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/editor/GravatarQuickEditorPage.kt

+                altText = altText,
+                avatarUrl = avatarUrl,
+                onBackPressed = { navController.popBackStack() },
+                modifier = Modifier.padding(16.dp),


❓ All other pages define their own paddings, rather than it being set here. Is there a reason you did it here?

AdamGrzybkowski · 2025-01-07T10:30:00Z

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextViewModel.kt

+            _uiState.update { currentState ->
+                currentState.copy(
+                    altText = newAltText,
+                    isSaveButtonEnabled = newAltText != altText,


When the newAltTest is empty the API call returns 200, but it doesn't update it. Looks like it quietly fails. Should we disable the save button in such cases?

hamorillo added new feature New feature [Feature] Gravatar-Quickeditor Gravatar Quick Editor module labels Dec 17, 2024

hamorillo added this to the 2.3.0 milestone Dec 17, 2024

hamorillo force-pushed the hamorillo/alt-text-404 branch from 9d28fbf to fe57213 Compare December 17, 2024 11:04

hamorillo commented Dec 17, 2024

View reviewed changes

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/components/AltTextSection.kt Outdated Show resolved Hide resolved

hamorillo requested a review from AdamGrzybkowski December 17, 2024 11:28

hamorillo marked this pull request as ready for review December 17, 2024 11:28

hamorillo force-pushed the hamorillo/alt-text-404 branch from fe57213 to 4dd4122 Compare December 17, 2024 11:51

AdamGrzybkowski reviewed Dec 17, 2024

View reviewed changes

hamorillo marked this pull request as draft December 17, 2024 16:09

hamorillo force-pushed the hamorillo/alt-text-404 branch 2 times, most recently from 9bc1b80 to 2f1ddaf Compare December 19, 2024 17:52

hamorillo commented Dec 19, 2024

View reviewed changes

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextSection.kt Outdated Show resolved Hide resolved

hamorillo marked this pull request as ready for review December 19, 2024 18:08

hamorillo force-pushed the hamorillo/alt-text-404 branch from 2f1ddaf to e2ae454 Compare December 20, 2024 08:53

hamorillo mentioned this pull request Dec 20, 2024

Quick Editor: Alt Text - Fix back handler after keyboard is opened #513

Open

AdamGrzybkowski reviewed Dec 20, 2024

View reviewed changes

hamorillo added 9 commits December 30, 2024 15:21

Add Alt Text option in the more options menu

5c98f0b

Add the AltText section to the NavHost in the QEEditorPage

84bcf76

Implement first iteration of AltTextSection

040a557

AltTextSection screenshot tests

e5b31af

Add loading option to the QEButton

4afc827

In the Figma file the height is fixed to 50.dp

Create AltTextSectionUiState to keep all data required to update the …

8280fb1

…AltText

Showing an error toast when the AltText update fails

6bade07

Rename AltTextSection to AltTextPage

550ee8d

Extension function to add the AvatarPicker graph

e262bc2

Use val with the avatarSize instead of the hardcode value

4ddf0d5

hamorillo force-pushed the hamorillo/alt-text-404 branch from 4e29afe to 3f3eb15 Compare December 30, 2024 14:28

github-advanced-security bot found potential problems Dec 30, 2024

View reviewed changes

gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextPage.kt Fixed Show fixed Hide fixed

hamorillo commented Dec 30, 2024

View reviewed changes

hamorillo mentioned this pull request Dec 30, 2024

Quick Editor: Alt Text - Sync data between AltTextViewModel and AvatarPickerViewModel #516

Open

hamorillo added 2 commits December 30, 2024 15:58

Split the AvatarPickerViewModel to create an idependant AltTextViewModel

21ace3f

Remove AltText related code from AvatarPickerViewModel

29c3a14

hamorillo force-pushed the hamorillo/alt-text-404 branch from 3f3eb15 to 29c3a14 Compare December 30, 2024 14:59

hamorillo requested a review from AdamGrzybkowski January 7, 2025 07:38

AdamGrzybkowski approved these changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick Editor - Alt text feature foundations #501

Quick Editor - Alt text feature foundations #501

hamorillo commented Dec 17, 2024 •

edited

Loading

wpmobilebot commented Dec 17, 2024 •

edited

Loading

AdamGrzybkowski left a comment •

edited

Loading

AdamGrzybkowski Dec 17, 2024

hamorillo Dec 17, 2024

hamorillo Dec 19, 2024 •

edited

Loading

AdamGrzybkowski Dec 20, 2024

hamorillo Dec 20, 2024

hamorillo Dec 30, 2024 •

edited

Loading

AdamGrzybkowski commented Dec 17, 2024

hamorillo commented Dec 19, 2024

AdamGrzybkowski left a comment

AdamGrzybkowski Dec 20, 2024

AdamGrzybkowski Dec 20, 2024

AdamGrzybkowski commented Dec 20, 2024

hamorillo Dec 30, 2024

AdamGrzybkowski left a comment

AdamGrzybkowski Jan 7, 2025

AdamGrzybkowski Jan 7, 2025

Quick Editor - Alt text feature foundations #501

Are you sure you want to change the base?

Quick Editor - Alt text feature foundations #501

Conversation

hamorillo commented Dec 17, 2024 • edited Loading

Description

Testing Steps

wpmobilebot commented Dec 17, 2024 • edited Loading

AdamGrzybkowski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamorillo Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamorillo Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

AdamGrzybkowski commented Dec 17, 2024

hamorillo commented Dec 19, 2024

AdamGrzybkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamGrzybkowski commented Dec 20, 2024

Choose a reason for hiding this comment

AdamGrzybkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamorillo commented Dec 17, 2024 •

edited

Loading

wpmobilebot commented Dec 17, 2024 •

edited

Loading

AdamGrzybkowski left a comment •

edited

Loading

hamorillo Dec 19, 2024 •

edited

Loading

hamorillo Dec 30, 2024 •

edited

Loading