Add new UI for selecting an attachment type #7429

jonnyandrew · 2022-10-21T11:38:21Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Add new UI for selecting an attachment type for users of the new rich text editor.
Also: add rounded corners to bottom sheets as per designs (note this currently only applies when bottom sheets are not in the fully expanded state - see open issues 1, 2).

Motivation and context

Screenshots / GIFs

Before	After

Tests

New attachment UI

Enable the rich text editor via labs
Enter a room
Tap the plus icon to start adding an attachment
Observe the new attachment screen
Tap an item to add an attachment
Observe the attachment flow starts

Old attachment UI

Disable the rich text editor via labs
Enter a room
Tap the plus icon to start adding an attachment
Observe the existing inline attachment UI

Tested devices

Physical
Emulator
OS version(s): 21, 33

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

Note these are currently only visible in the collapsed state. - [Google issue](https://issuetracker.google.com/issues/144859239) - [Rejected PR](material-components/material-components-android#437) - [Github issue](material-components/material-components-android#1278)

bmarty

Nice work, thanks!
Just a few minor remarks.

bmarty · 2022-10-24T05:32:15Z

library/ui-styles/src/main/res/values/styles_bottom_sheet.xml

@@ -14,6 +14,7 @@
        <!-- Default color for text View -->
        <item name="android:textColorTertiary">@color/element_content_primary_light</item>
        <item name="android:textColorLink">@color/element_link_light</item>
+        <item name="bottomSheetStyle">@style/BottomSheetStyle</item>


This change impacts all the BottomSheet of the app, did you check it does not break the other ones?

2938112

I think it might be better to update the global bottom sheet style in a separate PR. Given this change hasn't even fully met the design requirements (only affects the non-expanded state), it's not a big loss to remove for now.

bmarty · 2022-10-24T05:43:34Z

vector/src/main/java/im/vector/app/core/ui/views/BottomSheetActionButton.kt

+    override fun setOnClickListener(l: OnClickListener?) {
+        super.setOnClickListener(l)
+        views.bottomSheetActionClickableZone.setOnClickListener(l)
+    }


I understand this is to fix the click effect?
I think this is not necessary to call super.setOnClickListener(l).

Not just for the click effect - for some reason setOnClickListener was not working unless we set it to the child view.

9da575b

bmarty · 2022-10-24T05:48:45Z

vector/src/main/java/im/vector/app/features/attachments/AttachmentTypeSelectorBottomSheet.kt

+    private val viewModel: AttachmentTypeSelectorViewModel by fragmentViewModel()
+    private val timelineViewModel: TimelineViewModel by parentFragmentViewModel()
+    private val sharedActionViewModel: AttachmentTypeSelectorSharedActionViewModel by viewModels(
+            ownerProducer = { requireParentFragment() }


bmarty · 2022-10-24T05:51:27Z

vector/src/main/java/im/vector/app/features/attachments/AttachmentTypeSelectorBottomSheet.kt

+
+    override fun invalidate() = withState(viewModel, timelineViewModel) { viewState, timelineState ->
+        super.invalidate()
+        views.location.visibility = if (viewState.isLocationVisible) View.VISIBLE else View.GONE


You can you this handy extension which have the same effect

views.location.isVisible = viewState.isLocationVisible

The code will be much clearer.

FTR isGone and isInvisible also exist.

Thanks for letting me know about this!

74591f5

bmarty · 2022-10-24T05:53:36Z

.../main/java/im/vector/app/features/attachments/AttachmentTypeSelectorSharedActionViewModel.kt

+class AttachmentTypeSelectorSharedActionViewModel @Inject constructor() :
+        VectorSharedActionViewModel<AttachmentTypeSelectorSharedAction>()
+
+sealed class AttachmentTypeSelectorSharedAction : VectorSharedAction {


We now prefer using sealed interface, but the whole codebase has not been migrated yet.

bmarty · 2022-10-24T05:55:24Z

vector/src/main/java/im/vector/app/features/attachments/AttachmentTypeSelectorViewModel.kt

+
+    init {
+        setState {
+            this.copy(


this. is not necessary.

bmarty · 2022-10-24T05:57:28Z

vector/src/main/res/layout/bottom_sheet_attachment_type_selector.xml

@@ -0,0 +1,91 @@
+<?xml version="1.0" encoding="utf-8"?>
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"


This LinearLayout seems useless, can you remove it please?

bmarty · 2022-10-24T06:00:32Z

library/ui-strings/src/main/res/values/strings.xml

+    <string name="attachment_type_selector_location">Location</string>
+    <string name="attachment_type_selector_camera">Camera</string>
+    <string name="attachment_type_selector_contact">Contact</string>
+    <string name="attachment_type_selector_text_formatting">Text formatting</string>


Lint is complaining that this is not used. If this is for future usage, you can ignore the error here, so that the string can get translated. Else please remove the string.

I've removed it for now so that we don't forget to remove any lint ignore rule later

0650b6a

This reverts commit 17c43c9.

bmarty

Thanks for the update!
You can merge the PR when you want.

jonnyandrew marked this pull request as ready for review October 21, 2022 12:56

jonnyandrew requested review from a team and fedrunov and removed request for a team and fedrunov October 21, 2022 13:00

jonnyandrew added 3 commits October 21, 2022 17:40

Add new attachments selection dialog

bec7143

Add changelog entry

f52aaef

bmarty reviewed Oct 24, 2022

View reviewed changes

jonnyandrew added 7 commits October 24, 2022 09:19

Remove redundant call to superclass click listener

9da575b

Refactor to use view visibility helper

74591f5

Change redundant sealed class to interface

c401d55

Remove unused string

0650b6a

Revert "Add rounded corners to bottom sheet dialog."

2938112

This reverts commit 17c43c9.

Remove redundant view group

b9c1aa6

Remove redundant this

cdc8f7f

jonnyandrew requested review from bmarty and a team October 24, 2022 08:44

bmarty approved these changes Oct 25, 2022

View reviewed changes

jonnyandrew merged commit 7cc06fa into element-hq:develop Oct 25, 2022

jonnyandrew mentioned this pull request Oct 26, 2022

[Rich text editor] Add plain text mode and new attachment UI #7459

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new UI for selecting an attachment type #7429

Add new UI for selecting an attachment type #7429

jonnyandrew commented Oct 21, 2022 •

edited

Loading

bmarty left a comment

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty Oct 24, 2022

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty Oct 24, 2022

jonnyandrew Oct 24, 2022

bmarty left a comment

		@@ -0,0 +1,91 @@
		<?xml version="1.0" encoding="utf-8"?>
		<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"

Add new UI for selecting an attachment type #7429

Add new UI for selecting an attachment type #7429

Conversation

jonnyandrew commented Oct 21, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

New attachment UI

Old attachment UI

Tested devices

Checklist

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

jonnyandrew commented Oct 21, 2022 •

edited

Loading