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(ui): Use AutoMirrored icons and migrated deprecated components #1631

Closed
wants to merge 5 commits into from

Conversation

BobbyESP
Copy link
Contributor

@BobbyESP BobbyESP commented Aug 12, 2024

This PR updates several icons to use their AutoMirrored counterparts for better right-to-left language support. It also updates all the deprecated UI components to use their new implementations.

HorizontalPagerIndicator still needs to be migrated, but Google hasn't updated it, so an own-implementation may be needed

BobbyESP and others added 3 commits August 12, 2024 17:26
This commit updates several icons to use their AutoMirrored counterparts for better right-to-left language support
This commit migrates the UI components that has been deprecated in the latest versions of Material 3:

- Replaced `Divider`
 with `HorizontalDivider`.
- Updated `ExposedDropdownMenuBox` to use `MenuAnchorType`.
- Added `progress` lambda to `CircularProgressIndicator`.
- Updated `HorizontalPagerIndicator` syntax.
- Added animation to `DialogCheckBoxItem`.
- Updated `LinearProgressIndicator` syntax.

- Replaced `AlertDialog` with `BasicAlertDialog`.
- Replaced `ClickableText` with `Text` and `AnnotatedString` with links.

---
- HorizontalPagerIndicator has to be updated
@JunkFood02
Copy link
Owner

HorizontalPagerIndicator still needs to be migrated

why?

@BobbyESP
Copy link
Contributor Author

HorizontalPagerIndicator still needs to be migrated

why?

It's not necessary, but deprecated by the Accompanist library (like almost everything in there)

Copy link
Owner

@JunkFood02 JunkFood02 left a comment

Choose a reason for hiding this comment

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

Also, "removing all deprecated warning" doesn't look like a trivial issue that could be covered by a single PR

@@ -212,22 +213,27 @@ fun AutoUpdateUnavailableDialog(onDismissRequest: () -> Unit = {}) {
)

val annotatedString = buildAnnotatedString {
append(text)
append(text.substring(0, text.indexOf(hyperLinkText)))
Copy link
Owner

Choose a reason for hiding this comment

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

what if the hyperLinkText is not at the end of the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the hyperLinkText is being a parameter for another string there is no other way than doing a substring for putting the style and click listening in the new implementation.

In this case I believe that the current implementation is well done. Modifying the string resource is not viable. Any ideas are welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only thing I think that might be okay, but tell me some other options If you are not comfortable with the changes and I'll do them. Thanks!

    val annotatedString = buildAnnotatedString {
        append(text.substring(0, text.indexOf(hyperLinkText)))

        withLink(
            LinkAnnotation.Clickable(
                tag = "Link to the latest app release in GitHub",
                styles = TextLinkStyles(
                    SpanStyle(
                        color = MaterialTheme.colorScheme.tertiary,
                        textDecoration = TextDecoration.Underline,
                    )
                ),
                linkInteractionListener = { _ ->
                    uriHandler.openUri("https://github.com/JunkFood02/Seal/releases/latest")
                    hapticFeedback.performHapticFeedback(HapticFeedbackType.LongPress)
                }
            )
        ) {
            append(hyperLinkText)
        }
    }

Copy link
Owner

Choose a reason for hiding this comment

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

not really understand what you meant on this:

  1. There's an addLink() which is a drop-in replacement of the addUrlAnnotation here
  2. If you'd go with withLink(), you should append the rest of the text which is after the hyperLinkText (if exist)
        append(text.substring(0, startIndex))
        //...
        append(hyperLinkText)
        //...
        append(text.substring(endIndex, text.length))

}
)
) {
append(text.substring(startIndex, endIndex))
Copy link
Owner

Choose a reason for hiding this comment

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

ditto

@BobbyESP BobbyESP closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants