KOA-4919 Add compose BpkDialog #1043
Conversation
| modifier = Modifier.padding(BpkSpacing.Base), | ||
| verticalArrangement = Arrangement.spacedBy(BpkSpacing.Base), | ||
| ) { | ||
| var dialogShown by rememberSaveable { mutableStateOf<Dialog?>(null) } |
There was a problem hiding this comment.
I think this is a good example for why we shouldn't handle the state within our component/decide how state is stored as it will depend on usage - some may also want to let the viewmodel handle the state etc - i'd rather focus on education around remember vs rememberSaveable than enforce it as essentially it isn't a state internal to the component - it's a state that decides if a component should exist or not
marianeum
force-pushed
the
KOA-4919-dialog
branch
4 times, most recently
from
84d3f1d
to
8682de7
Compare
| } | ||
|
|
||
| when (dialogShown) { | ||
| Dialog.SuccessOneButton -> SuccessOneButtonDialogExample { dialogShown = null } |
There was a problem hiding this comment.
Can we rename it to showing dialogue type, and add an additional option for None? It'll make the code look cleaner
There was a problem hiding this comment.
that'll make the creation of them more difficult as the enum is also there for the button creation
| DialogButtons(buttons) | ||
| } | ||
| } | ||
| icon?.let { DialogIcon(icon = icon) } |
There was a problem hiding this comment.
Can we move this check inside DialogIcon?
| } | ||
| } | ||
|
|
||
| private val IconShape = RoundedCornerShape(percent = 50) |
marianeum
force-pushed
the
KOA-4919-dialog
branch
from
8682de7
to
b170f9a
Compare
| } | ||
| } | ||
|
|
||
| private val CircleShape = RoundedCornerShape(percent = 50) |
There was a problem hiding this comment.
There's a CircleShape in Material library you can use instead :)
alert dialog is a loaded term on Android
marianeum
force-pushed
the
KOA-4919-dialog
branch
from
b170f9a
to
5e6cab2
Compare
| } | ||
| } | ||
|
|
||
| private fun record(content: @Composable () -> Unit) { |
There was a problem hiding this comment.
as dialogs are in a separate window we need to run the screenshot tests slightly different
| } | ||
| } | ||
|
|
||
| composeTestRule.onNode(isDialog()).assertIsDisplayed() |
There was a problem hiding this comment.
we need to add the assertion here to wait for the dialog to be shown
| } | ||
|
|
||
| // we need this to be able to get the dialog root, rather than the window root | ||
| private fun getViewRoots(): List<ViewGroup> { |
There was a problem hiding this comment.
gotta love some reflection 😬
| ViewScreenshot("Calendar 2 - Pre-selected range", "range", ::setupCalendar2), | ||
| ViewScreenshot("Calendar 2 - Day colours", "colored", ::setupCalendar2), | ||
| ViewScreenshot("Calendar 2 - Day labels", "labeled", ::setupCalendar2), | ||
| ViewScreenshot("Calendar - Default", "range") { setupCalendar() }, |
There was a problem hiding this comment.
we now have a property passed through here for compose screenshots as they don't have the static api that the old view espresso tests use, so can't use the lambda API anymore
| @@ -170,6 +178,10 @@ private fun setupDialog() { | |||
| Thread.sleep(50) | |||
| } | |||
|
|
|||
| private fun setupComposeDialog(testRule: ComposeTestRule, dialog: ShownDialog) { | |||
There was a problem hiding this comment.
need to click the button for the dialog to show
| title: String, | ||
| text: String, | ||
| confirmButton: DialogButton, | ||
| secondaryButton: DialogButton, |
There was a problem hiding this comment.
this enforces that if there's three buttons the secondary button is always set
| } | ||
|
|
||
| @Composable | ||
| fun BpkDestructiveDialog( |
There was a problem hiding this comment.
the destructive dialog is not available with three buttons
| icon = icon?.let { Dialog.Icon.Success(icon) }, | ||
| title = title, | ||
| text = text, | ||
| buttons = listOfNotNull( |
There was a problem hiding this comment.
externally we want to enforce the types of buttons & order, but internally we can be flexible to have a simpler API
Remember to include the following changes:
README.mdIf you are curious about how we review, please read through the code review guidelines