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: Improve Reaction UI #1490

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

Conversation

jamesarich
Copy link
Contributor

@jamesarich jamesarich commented Dec 19, 2024

Moves the ReactionRow below the MessageItem within a Column using negative padding to provide slight overlap.

  • Updates ReactionItem UI to a more modern style with borders and rounded corners.
  • Implements an ellipsis indicator for overflowing reactions, allowing users to expand and view more.
  • Changes the reaction button to an icon that includes a plus, indicating the ability to add a reaction.
Screen_recording_20241219_104048.mp4

jamesarich and others added 3 commits December 19, 2024 10:26
…elow the MessageItem within a Column, providing better visual separation.

- Updates ReactionItem UI to a more modern style with borders and rounded corners.
- Implements an ellipsis indicator for overflowing reactions, allowing users to expand and view more.
- Changes the reaction button to a plus icon, indicating the ability to add a reaction.
Changed the reaction button border color from `onSurface` to `secondaryVariant` to improve visual clarity.
Copy link
Member

@andrekir andrekir left a comment

Choose a reason for hiding this comment

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

One small change and should be ready to merge.

)
Column(
modifier = Modifier.padding(vertical = 8.dp),
verticalArrangement = Arrangement.spacedBy((-12).dp)
Copy link
Member

Choose a reason for hiding this comment

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

Avoid adding Columns inside LazyColumns, use spacing modifiers like padding or offset directly in ReactionRow where it's needed.

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 couldn't find much documentation on why this should be avoided - but it was easy enough to swap out for offset and zindex.

- Adds modifier parameter to ReactionRow for styling flexibility.
- Removes Column and adjusts layout in MessageList for better message spacing and reaction positioning.
- Adds zIndex and offset to reaction row for improved visual layering.
@jamesarich jamesarich requested a review from andrekir January 3, 2025 13:25
Copy link
Member

@andrekir andrekir left a comment

Choose a reason for hiding this comment

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

the reaction placement doesn't look right.

Screen_Recording_20250104_105825_Meshtastic.mp4

@jamesarich
Copy link
Contributor Author

the reaction placement doesn't look right.

Screen_Recording_20250104_105825_Meshtastic.mp4

wow i didn't even catch that it was the row above... 😮‍💨
image

@jamesarich
Copy link
Contributor Author

@andrekir - after a lot of trial and error I think I found a solution that avoids using a nested Column... it does however need to calculate the padding based on the message height, which feels terribly hacky to me.

I never did find a very great argument against nesting a Column within a LazyColumn item. When I asked Gemini:

Nesting a Column inside a LazyColumn like that is generally acceptable and is a common pattern in Jetpack Compose.
The LazyColumn composable is designed for efficiently displaying a large number of items by only composing and laying out the visible items. Each item within the LazyColumn's items block is individually composed, and it's perfectly fine to use a Column or other layout composables to structure the content of each item.
In your example, the Column is used to arrange the MessageItem and the FlowRow vertically within each item of the LazyColumn. This is a good way to structure your UI and it won't lead to the issues associated with nesting two lazy lists.

I'm inclined to revert to the Column implementation unless you have a different solution in mind or you're ok with this padding calculation.

@jamesarich jamesarich requested a review from andrekir January 4, 2025 17:27
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.

2 participants