Space explore rooms screen alignment with design in figma #5834

fedrunov · 2022-04-25T13:07:20Z

Type of change

Feature
Bugfix
Technical
Other

Content

current implementation of "Explore Space" screen doesn't match Figma design

Motivation and context

Screenshots / GIFs

Before	After

Tests

open app drawer (left panel)
click on any space menu button
open space explore scree

Tested devices

Physical
Emulator
OS version(s):

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()

github-actions · 2022-04-25T13:41:31Z

Unit Test Results

122 files ±0 122 suites ±0 2m 8s ⏱️ -17s
205 tests ±0 205 ✔️ ±0 0 💤 ±0 0 ❌ ±0
690 runs ±0 690 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit dc8c7b1. ± Comparison against base commit 424fb55.

♻️ This comment has been updated with latest results.

Florian14

The code seems ok, just some small feedback.
Also, in your screenshot, I see a search icon instead of the filter icon, but in the code, it looks ok, can you confirm?

Florian14 · 2022-05-06T07:43:07Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

-                                    else          -> host.stringProvider.getString(R.string.action_join)
+                if (filteredChildInfo.isEmpty()) {
+                    spaceDirectoryFilterNoResults {
+                        id("no results")


it probably works but I don't see any existing id containing spaces inside so I'd suggest not using spaces in ids

there is also one in the same file :)

id("HS no Support")

but I agree it feels weird, so I changed both to be consistent with majority in the project

Florian14 · 2022-05-06T07:54:06Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

+            return filter.split(" ").all {
+                spaceChildInfo.name?.contains(it, ignoreCase = true).orFalse() ||
+                        spaceChildInfo.topic?.contains(it, ignoreCase = true).orFalse()
+            }


As I understand, if the filter is "Jo Do", it will also match "DoeJohn", is that expected? Should we take care of the ordering of the matches?

I think it's ok to not take ordering in account since there should not be so many items after filter is applied. Also that's actually a copy-paste and this is 4th copy of this code, so I think I'll create a small refactoring issue for this later.

Do we have an issue to track this refactoring change?

I've just found that there is exactly same implementation of SpaceChild MatchFilter, so I've reused that instead of copy paste.

And about issue - I'm not sure if it's actually could be refactored, so I decided to take a look on that after this will be merged and decide what to do

IMO it should be aligned with the other platforms to have the same results, so I agree it would be the object of a dedicated PR.
The same issue exists about the search in the user directory which leads to different results depending on the platform.

# Conflicts: # vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

ericdecanini

As Florian had pointed out, the implementation using the search icon looks different from the design in the issue which has the filter icon. If this was communicated somewhere, can we get some visibility on it? Furthermore, as per the screenshots above, I don't have the standard three-dot menu icon. As I'm not too familiar with this page, are there scenarios where this is expected?

ericdecanini · 2022-05-12T10:22:40Z

changelog.d/5658.misc

@@ -0,0 +1 @@
+space explore UI is aligned with design in Figma


If these are to be used as release notes then I think it's better to:

Tag this as .feature rather than .misc (from a user perspective this is a feature change more than a misc or bugfix)

Describe the design change itself instead of relating it back to figma (which external users/contributors may not have access to)

ericdecanini · 2022-05-12T10:24:27Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

+            return filter.split(" ").all {
+                spaceChildInfo.name?.contains(it, ignoreCase = true).orFalse() ||
+                        spaceChildInfo.topic?.contains(it, ignoreCase = true).orFalse()
+            }


Do we have an issue to track this refactoring change?

ericdecanini · 2022-05-12T10:30:58Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

@@ -196,4 +209,20 @@ class SpaceDirectoryController @Inject constructor(
            }
        }
    }
+
+    class RoomSearchMatchFilter : Predicate<SpaceChildInfo> {


We try and keep only one class per file (as per #5817 (comment))

I've removed this duplicate and reused existing implementation

ericdecanini · 2022-05-12T10:32:51Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryViewModel.kt

        }
    }

+    private fun filter(query: String?) {
+        setState { copy(currentFilter = query ?: "") }


It's nicer to use query.orEmpty() here

fedrunov · 2022-05-13T08:45:42Z

Also, in your screenshot, I see a search icon instead of the filter icon, but in the code, it looks ok, can you confirm?

As Florian had pointed out, the implementation using the search icon looks different from the design in the issue which has the filter icon. If this was communicated somewhere, can we get some visibility on it?

When SearchView is inflated as action view for menu, it doesn't receive styled attrs from xml so all icons are default. And there is no way to set this icons for SearchView on runtime. We could do a custom View or try custom ActionProvider, but it looks like overkill for changing one icon.

Furthermore, as per the screenshots above, I don't have the standard three-dot menu icon. As I'm not too familiar with this page, are there scenarios where this is expected?

three-dot appears if there is any item to be shown. Currently there is only one item - add room, and it could be hidden if you can't add room to this space. In that case entire three-dot menu will not be displayed

… match filter replaced with existing implementation, small code style tweaks

ericdecanini

Thanks for the clarification. LGTM

Florian14 · 2022-05-16T11:38:22Z

Also, in your screenshot, I see a search icon instead of the filter icon, but in the code, it looks ok, can you confirm?

As Florian had pointed out, the implementation using the search icon looks different from the design in the issue which has the filter icon. If this was communicated somewhere, can we get some visibility on it?

When SearchView is inflated as action view for menu, it doesn't receive styled attrs from xml so all icons are default. And there is no way to set this icons for SearchView on runtime. We could do a custom View or try custom ActionProvider, but it looks like overkill for changing one icon.

I did a similar change for a fork, I confirm that you cannot change the default icon for a specific SearchView (except by changing it for all the SearchView in the application, which is not the goal). So, I had to do it programmatically in the "onPrepareOptionsMenu" and it worked great as far as I know.

I don't think it is a blocker for this PR but it'd be great to have the correct icon regarding the performed action at the end: Is a search or a filter applied to the room list?

bmarty

Just one remark, else LGTM (static review)

bmarty · 2022-05-16T12:30:40Z

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

@@ -0,0 +1,36 @@
+<?xml version="1.0" encoding="utf-8"?>
+<androidx.appcompat.widget.LinearLayoutCompat xmlns:android="http://schemas.android.com/apk/res/android"


I think it's not necessary to explicitly use LinearLayoutCompat, LinearLayout is OK. I think LinearLayoutCompat will be inflated at runtime by AppCompatViewInflater anyway (but after checking this file I may be wrong).

* develop: Space explore rooms screen alignment with design in figma (#5834) leaving space aligned with ios (#5942) Fix usage of System.currentTimeMillis(). This a bit mocky but anyway it's better to use SystemClock.elapsedRealtime() for this case.

* develop: (251 commits) Space explore rooms screen alignment with design in figma (#5834) leaving space aligned with ios (#5942) Fix usage of System.currentTimeMillis(). This a bit mocky but anyway it's better to use SystemClock.elapsedRealtime() for this case. Update TimelinePreviousLastForwardTest.kt Changelog.d Rename 'getDomain' to 'getServerName'. Well-known lookups should not include the port of a server. Ah it was used in Gplay variant. Anyway I think we can still remove this now. Fix parsing of location data in non encrypted room Additionally increment for TimelinePreviousLastForwardTest Notify other devices of acceptance of verification request Instead of using a magic number, explicitly test for the events we expect. Adding comments on some strings and removing non necessary plural Remove ShortcutBadger lib and usage (it was dead code) Version++ Fastlane file Towncrier Add `.login` to get the username of the PR merger. Rename folder for the PlayStore Remove empty translations ... # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/SessionModule.kt

github-actions · 2022-05-19T11:58:52Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=20 failures=0 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=55 failures=2 errors=0 skipped=1
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

…#5834)

Space explore rooms screen redisign

c8ad1d2

fedrunov changed the title ~~Space explore rooms screen redisign~~ Space explore rooms screen alignment with design in figma Apr 25, 2022

fedrunov requested review from a team, bmarty, Florian14 and ericdecanini and removed request for a team May 5, 2022 11:51

Florian14 reviewed May 6, 2022

View reviewed changes

NIkita Fedrunov added 2 commits May 11, 2022 12:29

Merge branch 'develop' into feature/nfe/space_explore_ui_alignment

e05df2b

# Conflicts: # vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

code review changes

a403c4a

fedrunov requested a review from Florian14 May 11, 2022 11:14

ericdecanini suggested changes May 12, 2022

View reviewed changes

changelog modifed to reflect exact work done, duplicate of room title…

05b50de

… match filter replaced with existing implementation, small code style tweaks

fedrunov requested a review from ericdecanini May 13, 2022 10:23

ericdecanini approved these changes May 13, 2022

View reviewed changes

bmarty approved these changes May 16, 2022

View reviewed changes

Merge branch 'develop' into feature/nfe/space_explore_ui_alignment

dc8c7b1

fedrunov merged commit a30912f into develop May 19, 2022

fedrunov deleted the feature/nfe/space_explore_ui_alignment branch May 19, 2022 09:49

piersonleo pushed a commit to piersonleo/element-android that referenced this pull request Jun 7, 2022

Space explore rooms screen alignment with design in figma (element-hq…

e0e0c9f

…#5834)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Space explore rooms screen alignment with design in figma #5834

Space explore rooms screen alignment with design in figma #5834

fedrunov commented Apr 25, 2022 •

edited

Loading

github-actions bot commented Apr 25, 2022 •

edited

Loading

Florian14 left a comment

Florian14 May 6, 2022

fedrunov May 11, 2022

Florian14 May 6, 2022

fedrunov May 11, 2022

ericdecanini May 12, 2022

fedrunov May 13, 2022

Florian14 May 16, 2022

ericdecanini left a comment

ericdecanini May 12, 2022

ericdecanini May 12, 2022

ericdecanini May 12, 2022

fedrunov May 13, 2022

ericdecanini May 12, 2022

fedrunov commented May 13, 2022 •

edited

Loading

ericdecanini left a comment

Florian14 commented May 16, 2022

bmarty left a comment

bmarty May 16, 2022

github-actions bot commented May 19, 2022

		@@ -0,0 +1 @@
		space explore UI is aligned with design in Figma

		@@ -0,0 +1,36 @@
		<?xml version="1.0" encoding="utf-8"?>
		<androidx.appcompat.widget.LinearLayoutCompat xmlns:android="http://schemas.android.com/apk/res/android"

Space explore rooms screen alignment with design in figma #5834

Space explore rooms screen alignment with design in figma #5834

Conversation

fedrunov commented Apr 25, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Apr 25, 2022 • edited Loading

Unit Test Results

Florian14 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

ericdecanini 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

fedrunov commented May 13, 2022 • edited Loading

ericdecanini left a comment

Choose a reason for hiding this comment

Florian14 commented May 16, 2022

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 19, 2022

Matrix SDK

Integration Tests Results:

fedrunov commented Apr 25, 2022 •

edited

Loading

github-actions bot commented Apr 25, 2022 •

edited

Loading

fedrunov commented May 13, 2022 •

edited

Loading