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

ANDROID-14792 Support Checkbox with Links for Compose implementation #400

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

haynlo
Copy link
Contributor

@haynlo haynlo commented Dec 19, 2024

🎟️ Jira ticket

ANDROID-14792

🥅 What's the goal?

Support Checkbox with Links for Compose implementation

⚠️ Don't panic! The actual changes affect only less than 10 files. All other changes are related to the baseline ScreenShot due to the migration of the RippleTheme.

🚧 How do we do it?

  • Update Compose UI version
  • Fix problemas related to Compose UI upgrade
    • RippleTheme is now deprecated. It has been migrated.
  • Apply Role.Checkbox to Row wrapper to improve component usability.
  • Change TextWithLinks Composable in order to support new spannableString withLink method

☑️ Checks

  • I updated the documentation, including readmes and wikis. If this is a breaking change, tag the PR with "Breaking Change" label and remember to include breaking change migration guide in release notes where this version is released.
  • Tested with dark mode.
  • Tested with API 24.
  • Sync done with iOS team for this feature to ensure alignment, if applies.
  • Accessibility considerations.

🧪 How can I test this?

Ripple effect still being the same after the migration

Ripple Before Ripple After
ripple_before.mp4
ripple_after.mp4

Demo of main change

CheckBox with links in Compose
demo_checkbox_with_links.mp4

Copy link

📱 New catalog for testing generated: Download

Copy link

📱 New catalog for testing generated: Download

compose_ui_version = "1.5.4"
compose_ui_version = "1.7.6"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update to the latest stable version in order to use the new withLink method.

Comment on lines 58 to +59
implementation "androidx.compose.material:material:$compose_ui_version"
implementation("androidx.compose.material:material-icons-extended:$compose_ui_version")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some ComposeUI Icons used in Mística have been moved into a separated dependency, so I've just added the new one to support the already used Icons.

Comment on lines 58 to +59

@OptIn(ExperimentalMaterialApi::class)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New RippleTheme has been deprecated. So there's an error when compiling the App.
There is an official migration guide to replace RippleTheme: https://developer.android.com/develop/ui/compose/touch-input/user-interactions/migrate-indication-ripple#migrate-ripple-theme

The migration involves using ExperimentalMaterialApi classes.

@@ -1,11 +1,15 @@
@file:OptIn(ExperimentalMaterialApi::class) // Required for RippleConfiguration usage
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 90% of the methods inside this class need use the new LocalRippleConfiguration so I've added the OptIn annotation in the file level to avoid add the same annotation for each method.

Comment on lines +28 to +30
@Composable
private fun getMisticaRippleConfiguration(color: Color, alpha: RippleAlpha? = null) =
RippleConfiguration(color, alpha ?: RippleDefaults.rippleAlpha(color, !isSystemInDarkTheme()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace deprecated RippleTheme into the new one recommended by Android, maintaining the previous configuration we had in Mistica.

Comment on lines +32 to +37
Row(modifier = Modifier.toggleable(
value = checked,
onValueChange = onCheckedChange,
role = Role.Checkbox
)) {
Checkbox(
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 one of the two main changes: Apply Role.Checkbox to Row wrapper to improve component usability.

)) {
Checkbox(
checked = checked,
onCheckedChange = null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -22,92 +22,51 @@ internal fun TextWithLinks(
modifier: Modifier = Modifier,
) {
val textLinkColour = MisticaTheme.colors.textLink
val linksText = remember { getAnnotatedLinksString(text = text, links = links, highlightColor = textLinkColour) }
ClickableText(
val linksText = remember { getAnnotatedLinksString(originalText = text, links = links, highlightColor = textLinkColour) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From here, you can see the second main change. The logic for creating the SpannableString has been redefined using the new withLink method.

}
}
) = buildAnnotatedString {
val linkMap = links.associateBy { it.link }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create a map for faster link lookup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a lot of screenshot changes due to Ripple migration, no need to review them.

@haynlo haynlo requested review from jeprubio, a team and yamal-alm and removed request for a team December 19, 2024 14:08
Copy link
Contributor

@jeprubio jeprubio left a comment

Choose a reason for hiding this comment

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

Great 👍

val (linkText, link) = linkEntry
withLink(
link = LinkAnnotation.Clickable(
tag = TAG,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if TextLink should include a tag so that it has this "TextWithLinks" default value but it can be changed if needed for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

this would be very useful specially when there are more than one link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here: f85d561

Copy link
Contributor

@yamal-alm yamal-alm left a comment

Choose a reason for hiding this comment

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

looks good 👍 This must be a new record 😄
Captura de pantalla 2024-12-19 a las 16 15 22

Copy link

📱 New catalog for testing generated: Download

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