-
Notifications
You must be signed in to change notification settings - Fork 759
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
Navigation | New Home tab to priorize all unread - BLOCKED #4010
Conversation
General thought: I don't see the need to separate rooms into categories like Favorites, Unread DM, Unread Rooms, DM, Rooms, and Low Priority. It just wastes valuable space which could be used to show a preview of the latest message. If this is supposed to fix #650, then it is not quite what people wanted.
|
0dbcce6
to
cf9a23c
Compare
30c763e
to
0316a15
Compare
d900132
to
914a8b0
Compare
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.
First remarks:
- This PR has to be rebased.
- Also please ensure that the sanity test is still passing (it will need some update)
- I will do a full review later
a0a1ee1
to
9fb4fcc
Compare
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.
Thanks for doing this, I'm sure it will be really appreciated.
Please see my comments
private fun handleViewResumed() { | ||
// Good place to check for layout mode | ||
setState { | ||
copy(layoutMode = vectorPreferences.getLayoutMode()) |
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.
setState only if the value is not the same? It's maybe done internally by MvRx, not sure.
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.
selectSubscribe is doing distinct until changed
// Layout | ||
|
||
fun getLayoutMode(): LayoutMode { | ||
return defaultPrefs.getString(SETTINGS_LAYOUT_MODE, LayoutMode.PRODUCTIVITY.name).let { |
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.
Default is PRODUCTIVITY? I would say SIMPLE would be better, or maybe reuse the setting SETTINGS_LABS_UNREAD_NOTIFICATIONS_AS_TAB
, if true -> PRODUCTIVITY
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 it was decided to use productivity as default to not disturb too much current experience and user flows.
It's a good idea to check for SETTINGS_LABS_UNREAD_NOTIFICATIONS_AS_TAB as a discriminator.
I also wonder if SETTINGS_LABS_UNREAD_NOTIFICATIONS_AS_TAB user will be satisifed by the new home, maybe they would want the new home to only show unreads?
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.
Ok, but new accounts should definitely see the SIMPLE layout then. Do you agree?
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 changed to do as you proposed (SETTINGS_LABS_UNREAD_NOTIFICATIONS_AS_TAB, if true -> PRODUCTIVITY)
@@ -0,0 +1,84 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
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.
rename the two drawable vectors to ensure they have a common prefix: ic_layout_pro
and ic_layout_simple
. Also those are not really icon, so maybe setting_layout_pro
and setting_layout_simple
would be better names
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.
done
<path | ||
android:pathData="M16,17m-8,0a8,8 0,1 1,16 0a8,8 0,1 1,-16 0" | ||
android:strokeWidth="1" | ||
android:fillColor="#C1C6CD" |
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.
does it render well in dark theme?
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.
In simple layout, there have not any Direct Messages button. I think it would be easier for searching people if there have a Direct Messages button.
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.
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
Sorry, but is it really what we want in dark theme? white bg like that?
EDIT: this is related to the same drawable, sorry
<!-- android:drawableEnd="@drawable/ic_pro_layout"--> | ||
<!-- android:text="@string/settings_home_layout_pro"--> | ||
<!-- tools:checked="false" />--> | ||
|
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.
cleanup?
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.
done
9fb4fcc
to
fe9b4ef
Compare
vector/src/main/java/im/vector/app/features/settings/VectorPreferences.kt
Outdated
Show resolved
Hide resolved
<?xml version="1.0" encoding="utf-8"?> | ||
<resources> | ||
|
||
<!-- room message colors --> |
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.
update this comment?
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, thanks!
As discussed, will be merged today so that we can get some feedback before the next release.
Is it already in the debug build? Cannot find it / give feedback |
The merging of this PR has been delayed, the changes it brings require more internal discussions |
Still, waiting for it. 🥺 Will it be available this year? |
We can only hope and be patient :) |
I understand that the Design Team might wand to unify the look&feel between Android&iOS, but this PR is really necessary for simplicity. |
Let's remove this PR, we will iterate with IA later. |
Can you please link the subsequent issue / PR? |
App Navigation modification.
This PR introduces a new Home Tab, in this tab you will find all important things you need to read first in a less busy and more compact layout so you can see more at once (one line per room with no last message preview or timestamp) .
It is combining rooms and people into one list, in order to stop people having to change tabs to check on unread rooms/people.
The Home tab contains in order
The existing DM tabs and Room Tabs are modified a bit, they won't show favorite or invites anymore as they will be on Home already. They will continue to display rooms in last activity order with a more detailed view (last message, typing indicator, timestamp, bigger avatar).