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

[ABW-1672] Add info popup #1169

Merged
merged 31 commits into from
Sep 7, 2024
Merged

[ABW-1672] Add info popup #1169

merged 31 commits into from
Sep 7, 2024

Conversation

giannis-rdx
Copy link
Contributor

@giannis-rdx giannis-rdx commented Aug 30, 2024

Description

This PR adds info buttons and info dialogs. It also:

  • adds a separate composable for warnings: WarningText
  • adds a separate composable for info buttons: InfoButton
  • added InfoDialog composable screen that can be triggers from several places in the wallet (similar to asset dialogs)
  • several UI fixes based on Zeplin

How to test

  1. Open Wallet and navigate to the screens that have a info button
  2. click on this and click also on the other links inside the dialog
  3. There are two types of links: web links that open the browser and the glossaryAnchors that update the content of the info dialog: See the video

Video

Screen_Recording_20240901_172302_Radix.Wallet.Dev.mp4

PR submission checklist

  • I have tested info buttons

@giannis-rdx giannis-rdx changed the title Feature/abw 1672 add info popup [ABW-1672] Add info popup Aug 30, 2024
@giannis-rdx giannis-rdx self-assigned this Aug 30, 2024
@giannis-rdx giannis-rdx marked this pull request as ready for review September 1, 2024 14:40
onDismiss = onDismiss
) {
InfoDialogContent(
scrollState = ScrollState(initial = 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rememberScrollState? I think this will also fix the issue of the scroll being reset (scrolled up) after clicking a URL and getting back to the app (or after pausing and resuming the app while the info pop up is displayed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this on purpose for the glossary items. For example, when you click on XRD it will scroll to the top so you can see the content from the beginning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, then to improve this and also fix the issue I described maybe it's worth using rememberScrollState and scrolling to the top based on a boolean (using LaunchedEffect) only when tapping on a glossary item and not a URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did ti with a slightly different way

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better!

Copy link
Contributor

@micbakos-rdx micbakos-rdx left a comment

Choose a reason for hiding this comment

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

The info links seem to have the wrong blue color. See an example here.

@@ -0,0 +1,34 @@
package com.babylon.wallet.android.presentation.dialogs.info

// the enums are defined the same way they are written in Strings ("glossaryAnchor" tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we need to define them that way? Does this have to do with serialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are planning to define them in Sargon, and plus to easily get the valueOf based on the glossary anchor value.

val context = LocalContext.current

val customHeading2: MarkdownComponent = {
val content = it.content
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused by it. Where does this come from?

Copy link
Contributor Author

@giannis-rdx giannis-rdx Sep 2, 2024

Choose a reason for hiding this comment

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

this comes from the composable
typealias MarkdownComponent = @Composable ColumnScope.(MarkdownComponentModel) -> Unit
it is the MarkdownComponentModel.
I updated this part

Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx 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!

@@ -132,7 +132,7 @@ private fun InfoDialogContent(
Column(
modifier = modifier
.background(RadixTheme.colors.defaultBackground)
.fillMaxHeight(0.5f)
.fillMaxHeight(0.9f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now it looks way better!

@@ -153,6 +156,10 @@ private fun InfoDialogContent(
override fun openUri(uri: String) {
if (uri.contains(GLOSSARY_ANCHOR)) {
onGlossaryItemClick(uri.drop(GLOSSARY_ANCHOR.length))
coroutineScope.launch {
// dialog updated with new glossary item therefore scroll to top
scrollState.animateScrollTo(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@giannis-rdx giannis-rdx force-pushed the feature/ABW-1672-add-info-popup branch from d9f1ebd to 977fc05 Compare September 6, 2024 08:04
Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx left a comment

Choose a reason for hiding this comment

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

If it's not a very difficult addition, it would be nice to display the title at the top of the info sheet when scrolling up and it's not visible anymore as part of the sheet content. This behavior is present on iOS and it looks good.

@giannis-rdx giannis-rdx force-pushed the feature/ABW-1672-add-info-popup branch from 8c33a55 to 835fbb1 Compare September 6, 2024 13:07
@giannis-rdx giannis-rdx force-pushed the feature/ABW-1672-add-info-popup branch from bbb9b7e to 5b97361 Compare September 6, 2024 14:34
Copy link

sonarcloud bot commented Sep 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

@giannis-rdx giannis-rdx merged commit a05ffec into main Sep 7, 2024
8 of 9 checks passed
@giannis-rdx giannis-rdx deleted the feature/ABW-1672-add-info-popup branch September 7, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants