-
Notifications
You must be signed in to change notification settings - Fork 155
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
Knocking : update create room flow #3804
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
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, just a few suggestions.
@@ -31,28 +37,87 @@ class CreateRoomDataStore @Inject constructor( | |||
field = value | |||
} | |||
|
|||
fun getCreateRoomConfig(): Flow<CreateRoomConfig> = combine( | |||
val createRoomConfig: Flow<CreateRoomConfig> = combine( |
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.
Maybe rename to createRoomConfigWithInvites
?
...room/impl/src/main/kotlin/io/element/android/features/createroom/impl/CreateRoomDataStore.kt
Show resolved
Hide resolved
Box( | ||
modifier = modifier | ||
.size(30.dp) | ||
.clip(RoundedCornerShape(8.dp)) | ||
.background(ElementTheme.colors.bgSubtleSecondary) | ||
.padding(3.dp), | ||
contentAlignment = Alignment.Center, |
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 have RoundedIconAtom
for this.
@PreviewsDayNight | ||
@PreviewWithLargeHeight |
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'm not sure if we can combine preview annotations like this 🫤 . In fact, I don't see a 'long' screenshot being recorded. We'd have to manually add @Preview(height = ...)
and @Preview(uiMode = Configuration.UI_MODE_NIGHT_YES, height = ...)
to it instead if I'm not mistaken.
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.
Yes, I've removed it in the last commits. I'll fix it in another PR with the final design
sealed interface RoomAddress { | ||
data class AutoFilled(val address: String) : RoomAddress | ||
data class Edited(val address: String) : RoomAddress | ||
} |
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.
👌
modifier | ||
.fillMaxWidth() | ||
.selectable( | ||
selected = isSelected, | ||
onClick = { onOptionClick(roomAccessItem) }, | ||
role = Role.RadioButton, | ||
) |
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.
One day we have to debug why your formatter does this 😓 .
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 agree it's polluting our PRs.
fun RoomPrivacyOption( | ||
roomPrivacyItem: RoomPrivacyItem, | ||
onOptionClick: (RoomPrivacyItem) -> Unit, | ||
fun RoomVisibilityOption( |
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.
Now that I think about it, maybe this should use a ListItem
inside instead?
@Composable | ||
fun RoomAccessOption( | ||
roomAccessItem: RoomAccessItem, | ||
onOptionClick: (RoomAccessItem) -> Unit, | ||
modifier: Modifier = Modifier, | ||
isSelected: Boolean = false, | ||
) { | ||
Row( |
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.
This should probably use a ListItem
too.
Quality Gate passedIssues Measures |
Content
This PR updates the create room flow with the following changes :
There is a new feature flag to enable to see the updated flow.
Motivation and context
Handles some parts of #3689
Screenshots / GIFs
Tests
Tested devices
Checklist