-
Notifications
You must be signed in to change notification settings - Fork 18
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
Alt text: Alt text counter #529
Conversation
📲 You can test the changes from this Pull Request in Gravatar Demo by scanning the QR code below to install the corresponding build.
|
gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextPage.kt
Show resolved
Hide resolved
gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextPage.kt
Show resolved
Hide resolved
gravatar-quickeditor/src/main/java/com/gravatar/quickeditor/ui/alttext/AltTextPage.kt
Show resolved
Hide resolved
val isSaveButtonEnabled: Boolean | ||
get() = altText != initialAltText && !isUpdating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to set this manually, so I moved it here and set it conditionally.
viewModel.onEvent(AltTextEvent.AvatarAltTextSaveTapped) | ||
advanceUntilIdle() | ||
|
||
viewModel.uiState.test { | ||
skipItems(2) | ||
expectMostRecentItem() | ||
viewModel.onEvent(AltTextEvent.AvatarAltTextSaveTapped) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bit more future-proof as we are only testing the states after AvatarAltTextSaveTapped
event without skipping fixed numbers of states.
a1b1423
to
ea42255
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a couple of things while testing:
- When I hit 0 in the counter, move the caret to the middle of the paragraph and start typing again, it erases the end of the text. Is that on purpose?
- This is maybe worth creating another issue but when adding multiple new lines at some point the "save" button disappears with no way to bring it back.
Screen_recording_20250110_144921.mp4
Not on purpose, but that's what the
Yes, this is unrelated. #533 |
That behavior can create some misunderstandings. However, I agree it's not easy to handle; for example, what if I have five left characters and I paste 20 characters in the middle? We have to consider those cases. For example, Revolut handles that with a negative counter. When you reach the limit, you are not allowed to add any more characters. |
I wouldn't complicate things too much. Here's my suggestion:
WDYT? |
Sounds good to me! I doubt we'll get many complaints about this obscure edge case anyway. |
ea42255
to
cc870a2
Compare
@mlumeau @hamorillo Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. I noticed a tiny weird thing, totally not blocking: the caret is still moving when trying to insert text at limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was writing a similar comment to Maxime's about the cursor moving without inserting any text. 🙂
@@ -188,7 +198,26 @@ internal fun AltTextPage( | |||
textStyle = LocalTextStyle.current.copy(color = MaterialTheme.colorScheme.onSurface), | |||
modifier = Modifier | |||
.fillMaxWidth() | |||
.padding(start = 16.dp, top = 8.dp, bottom = 8.dp), | |||
.fillMaxHeight(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏ I think we can replace it with fillMaxSize()
Yeah, I noticed that. The thing is we don't control the cursor position and to do that we have to keep that in the ViewModel. I did a quick try and it's a pain, to be honest. I will merge this for now and we can improve this later if we feel like it's a big issue. |
cc870a2
to
58cb184
Compare
58cb184
to
69bef4c
Compare
Closes #502
Description
This PR adds a little counter to show the remaining characters count for the alt text. Once the limit is reached the alt text won't be modified.
alt_text_limit.mp4
Testing Steps