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

Improvement: Redesign & restructure chat textfield #320

Merged
merged 25 commits into from
Jan 27, 2025

Conversation

julian-wls
Copy link
Contributor

@julian-wls julian-wls commented Jan 20, 2025

Problem Description

The Textfield used to create messages in the chat is currently not quite structured and looks rather outdated. Over time more and more buttons have been placed onto the textfield, which now makes it look quite overwhelming to the users. Formatting options placed below the OutlinedTextfield are not intuitive and inconsistent.

Changes

This PR applies the following changes to both, the UnfocusedPreviewReplyTextField and the CreateReplyUi:

  • Instead of showing a disabled send button on the UnfocusedPreviewReplyTextField a new button has been placed to allow users to upload images or attachments without activating the textfield.
  • The formatting options are now only visible clicking on a button to provided more structure.
  • A new button has been added allowing users to tag channels, users, exercises and lectures by selecting the option in a dropdown menu.
  • Users have the option to add strikethrough text formatting now.
  • The textfield has been redesigned and has a max height of 8 text lines before it becomes scrollable.
  • The AutocompletePopup has been redesigned to show icons for the exercise type and channel type.

Steps for testing

  1. Click the new plus button and select image and attachment upload and verify its functionality
  2. Create a message and verify that the textfield becomes scrollable after 8 lines with a autoscroll action which keeps the cursor focused.
  3. Click the format button and verify that all formatting options appear and the preview/edit buttons stay visible
  4. Close the formatting options and click the new tagging button. Verify that after selecting an option only the corresponding entries are shown in the popup.
  5. Verify that the icons in the popup are shown correctly.
  6. Verify that everything else works as before.

Screenshots

CreateReplyTextfieldNew
UnfocusedTextfieldNew

@julian-wls julian-wls self-assigned this Jan 20, 2025
@julian-wls julian-wls marked this pull request as ready for review January 23, 2025 15:09
@julian-wls julian-wls added the ready for review This PR can be reviewed label Jan 23, 2025
Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Omg, I can't tell you how much of an improvement this change is 😍 It's looking sooo much better now, thank you for addressing this!

On the functional side everything works as expected, I just have some minor points of discussion:

  1. In dark mode, the contrast for the Upload button in the unfocussedReplyField (with the + sign) is quite low, can we change this? (see screenshot)
  2. Very minor: when clicking the Upload button (+ sign), the DropdownMenu is a little bit too far to the top of the button imo. (see screenshot)
    image

The code comments are mostly just opportunities for refactorings btw :D

@FelberMartin
Copy link
Collaborator

FelberMartin commented Jan 24, 2025

Regarding the ProfilePictures not displaying in the Autocompletion hint dialog, I also just had a quick look, but it's very mysterious that it does not work. I think we should create a followup for this then 🤔

@julian-wls
Copy link
Contributor Author

Thanks a lot for the great feedback :)

I implemented the requested changes. I tried various approaches to reduce the space between the dropdown menu and the button you mentioned in your 2. point, however I did not find a working solution.
Regarding the plus sign I would suggest we treat it like any other white colors and turn it to black in the dark mode (use MaterialTheme.colorScheme.background). What's your opinion on this @FelberMartin:

ddddd

I think creating a follow up to show ProfilePictures in the AutocompletionPopup for users is a really good idea. I couldn't find a working solution yet, either.

@FelberMartin
Copy link
Collaborator

I implemented the requested changes. I tried various approaches to reduce the space between the dropdown menu and the button you mentioned in your 2. point, however I did not find a working solution.

Okay, I think its fine, not a major issue :)

Regarding the plus sign I would suggest we treat it like any other white colors and turn it to black in the dark mode (use MaterialTheme.colorScheme.background). What's your opinion on this @FelberMartin

Sounds and looks awesome 👍

Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Thank you for incorporating my change requests, the code looks very readable now :D

Regarding the two remaining open conversations, I found a solution after some debugging:

  • Remove .consumeWindowInsets(WindowInsets.systemBars) from SinglePageConversationBody
  • Remove navigationBarsPadding() from ConversationThreadScreen:88 and ConversationChatListScreen:131
  • Replace padding(bottom = WindowInsets.systemBars.asPaddingValues().calculateBottomPadding()) with navigationBarsPadding() in ReplyTextField. This did previously not work as expected because of the misused .consumeWindowInsets(WindowInsets.systemBars) call further up the UI tree

With these changes the padding of the ReplyTextField should work properly :D

@julian-wls
Copy link
Contributor Author

julian-wls commented Jan 25, 2025

  • Remove .consumeWindowInsets(WindowInsets.systemBars) from SinglePageConversationBody
  • Remove navigationBarsPadding() from ConversationThreadScreen:88 and ConversationChatListScreen:131
  • Replace padding(bottom = WindowInsets.systemBars.asPaddingValues().calculateBottomPadding()) with navigationBarsPadding() in ReplyTextField. This did previously not work as expected because of the misused .consumeWindowInsets(WindowInsets.systemBars) call further up the UI tree

With these changes the padding of the ReplyTextField should work properly :D

I tried applying your suggested changes, but I found multiple issues:

When testing on devices that still use the old navigationBar we would get this result:
DDDDDD
And this gap shows up:
gap

I'd suggest we stay with the current result as it works perfectly fine with older devices and smaller screen sizes. I also just added weight to the Column around the textfield to prevent this from happening on smaller devices when the textfield is growing:
ggggg

It should work fine now.

Copy link
Contributor

@eylulnc eylulnc left a comment

Choose a reason for hiding this comment

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

This is looking really good and modern!

I have one point about the formating features:

Listing is not working properly when we go back and continue to add items.

Screen.Recording.2025-01-25.at.12.47.17.mov

@FelberMartin
Copy link
Collaborator

When testing on devices that still use the old navigationBar we would get this result:

Okay thats weird. Then let's merge this PR with these paddings, and I will try to have deeper look into it next week

@julian-wls
Copy link
Contributor Author

This is looking really good and modern!

I have one point about the formating features:

Listing is not working properly when we go back and continue to add items.

Screen.Recording.2025-01-25.at.12.47.17.mov

I just observed this behavior on develop as well. As this is unrelated to this PR I'd suggest we merge this PR and fix this issue afterwards.

@julian-wls julian-wls added ready to merge This PR can be merged and removed ready for review This PR can be reviewed labels Jan 27, 2025
@eylulnc
Copy link
Contributor

eylulnc commented Jan 27, 2025

This is looking really good and modern!

I have one point about the formating features:

Listing is not working properly when we go back and continue to add items.

Screen.Recording.2025-01-25.at.12.47.17.mov

I just observed this behavior on develop as well. As this is unrelated to this PR I'd suggest we merge this PR and fix this issue afterwards.

Really interesting, when I compare it with a different branch I did not see a problem, but sure we can check it out later.

@julian-wls
Copy link
Contributor Author

This is looking really good and modern!

I have one point about the formating features:

Listing is not working properly when we go back and continue to add items.

Screen.Recording.2025-01-25.at.12.47.17.mov

I just observed this behavior on develop as well. As this is unrelated to this PR I'd suggest we merge this PR and fix this issue afterwards.

Really interesting, when I compare it with a different branch I did not see a problem, but sure we can check it out later.

Check this issue I just created.

@FelberMartin
Copy link
Collaborator

Can confirm that this issue also happens on develop, so I will merge this PR now :)

@FelberMartin FelberMartin merged commit f2f99fa into develop Jan 27, 2025
5 checks passed
@FelberMartin FelberMartin deleted the improvement/redesign-markdown-textfield branch January 27, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants