Add right panel with Room information #365

MidhunSureshR · 2021-05-23T11:51:41Z

No description provided.

bwindels · 2021-05-31T07:49:01Z

So, I just tried it out, and it looks really nice!! Exciting to see this 😄 A few functional things I noticed:

it would be great if you switch room, that it keep the details panel open or closed, based on the current URL. See "open-room" in parseUrlPath in src/domain/navigation/index.js.
I'd put the button to open the right panel as a menu option in the menu that is already there, especially given there is not much functionality there yet. Also, I think ideally it would be a link rather than a button. You'll probably need to adjust the CSS to have the same styling for links with a given class than for menu option buttons.
I'm not sure about showing the room id as a fallback of the room alias... I think if there is no alias, we should not show the subtitle. It might be useful to still (always) show the room id somewhere small & grey though, just so you can relate the id to other things if needed, perhaps below encryption?

Haven't looked at the code recently, will do so later.

src/domain/session/room/RoomViewModel.js

src/domain/session/SessionViewModel.js

bwindels

Thank you for your changed @MidhunSureshR! This is shaping up nicely! I did a full code review, don't be intimidated by the many comments, most should be very easy to address.

src/platform/web/ui/session/room/RoomView.js

src/platform/web/ui/session/rightpanel/RoomInfoView.js

src/domain/navigation/index.js

src/domain/session/RoomGridViewModel.js

src/platform/web/ui/css/right-panel.css

src/platform/web/ui/css/themes/element/theme.css

src/domain/session/rightpanel/RoomInfoViewModel.js

src/domain/session/SessionViewModel.js

bwindels

Had a look at the styling as well, mostly looks good, just a few suggestions.

src/platform/web/ui/css/themes/element/theme.css

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

- Move related parameters closer together. - Remove unused parameter. Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

src/platform/web/ui/session/rightpanel/RoomDetailsView.js

bwindels

Thank you for addressing all the comments, just went over those and resolved accordingly. Came across one issue and a little remaining nit. I'll do another review of the whole thing (rather than just going over the comments as I did now), but I think we're mostly good to go.

src/domain/session/room/RoomViewModel.js

src/domain/session/rightpanel/RoomDetailsViewModel.js

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

bwindels · 2021-06-14T15:29:59Z

Can you also fix some of the lint warnings in your new code? The current warning count is 16 and it'd be good if it didn't go up.

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

bwindels

Looking great, found a few more things, but after that we're good to merge I think.

src/platform/web/ui/css/themes/element/theme.css

bwindels · 2021-06-15T08:44:58Z

src/platform/web/ui/css/layout.css

@@ -65,7 +72,7 @@ the layout viewport up without resizing it when the keyboard shows */
 .middle .close-middle { display: none; }
 /* mobile layout */
 @media screen and (max-width: 800px) {
-    .SessionView:not(.middle-shown) {
+    .SessionView:not(.middle-shown, .right-shown) {


selector lists in :not are not supported in IE apparently. Can you write this out?

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

bwindels · 2021-06-24T07:10:41Z

src/domain/session/RoomGridViewModel.js

@@ -183,6 +189,7 @@ export class RoomGridViewModel extends ViewModel {

 import {createNavigation} from "../navigation/index.js";
 import {ObservableValue} from "../../observable/ObservableValue.js";
+import { Segment } from "../navigation/Navigation.js";


Nit: only the imports needed for the tests should be below here, the others up at the top. You can also avoid the import by doing this.navigation.segment(...) fwiw.

bwindels · 2021-06-24T07:12:13Z

src/platform/web/ui/css/layout.css

@@ -72,7 +72,7 @@ the layout viewport up without resizing it when the keyboard shows */
 .middle .close-middle { display: none; }
 /* mobile layout */
 @media screen and (max-width: 800px) {
-    .SessionView:not(.middle-shown, .right-shown) {
+    .SessionView:not(.middle-shown) {


is the .right-shown not needed then in here?

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

- This will stop the code from throwing when opening /details on UnknownRoomView. Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

bwindels reviewed May 31, 2021

View reviewed changes

src/domain/session/room/RoomViewModel.js Outdated Show resolved Hide resolved

src/domain/session/SessionViewModel.js Outdated Show resolved Hide resolved

MidhunSureshR force-pushed the room-info branch from 1a7d8e1 to fae94bd Compare June 2, 2021 06:43

bwindels marked this pull request as ready for review June 2, 2021 10:42

bwindels suggested changes Jun 3, 2021

View reviewed changes

src/platform/web/ui/css/themes/element/theme.css Outdated Show resolved Hide resolved

src/platform/web/ui/css/themes/element/theme.css Outdated Show resolved Hide resolved

src/platform/web/ui/css/themes/element/theme.css Outdated Show resolved Hide resolved

MidhunSureshR force-pushed the room-info branch from fae94bd to b906d3c Compare June 3, 2021 15:37

MidhunSureshR requested a review from bwindels June 7, 2021 15:06

MidhunSureshR added 22 commits June 9, 2021 16:51

Add view and vm for RoomInformation

c7ba472

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Allow details to be child of room

c7fd0fa

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add code to toggle RoomInformation panel

986271d

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Create RoomInformationView on toggle

e39572b

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Put name of property in emitChange

96959a3

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Handle case when path.get() may be null

439910f

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Allow details to be child of rooms

98d8d44

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove right panel on grid update if needed

7e38c3e

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Dispose vm preemptively

ddaa9b4

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Extract method

c3333f5

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove unnecessary check

9a605cc

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add CSS to display sidebar to the right

0d11f85

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Fallback to canonical alias if no names found

191eb09

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add background for sidebar

0ea2843

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add room avatar to RoomInfoView

6f1b77b

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make room avatar larger in RoomInfoView

aece82d

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make display name bold

d502a7f

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Better styling

a23e2c3

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Create method to add sidebar element

0a4f8af

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add method to create avatar section

653fcbb

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Bring in icon assets

ee29d7f

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add avatar size for design

f42553f

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

MidhunSureshR added 9 commits June 9, 2021 16:51

Remove unwanted width

2c0176f

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make value multiple of 4

6086d15

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove margin from top

95512b5

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Refactor function

fa67c5e

- Move related parameters closer together. - Remove unused parameter. Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Increase height to make element more centered

4005d70

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make formatting consistent

dd9a19b

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Add rule to warn on wrong formatting

0c5d118

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make formatting consistent with the codebase

b2f2d51

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Undo spillover from earlier formatting havoc

e2443a8

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

MidhunSureshR force-pushed the room-info branch from 8514340 to e2443a8 Compare June 9, 2021 11:22

bwindels reviewed Jun 14, 2021

View reviewed changes

src/platform/web/ui/session/rightpanel/RoomDetailsView.js Outdated Show resolved Hide resolved

bwindels reviewed Jun 14, 2021

View reviewed changes

src/domain/session/room/RoomViewModel.js Outdated Show resolved Hide resolved

src/domain/session/rightpanel/RoomDetailsViewModel.js Outdated Show resolved Hide resolved

MidhunSureshR added 2 commits June 14, 2021 20:32

Rename method

e10b455

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove space after brace in two places

7b811aa

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

MidhunSureshR added 2 commits June 14, 2021 21:04

Unsubscribe on dispose

88a1e34

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Undo lint config change

e50b503

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

bwindels suggested changes Jun 15, 2021

View reviewed changes

MidhunSureshR added 5 commits June 15, 2021 14:30

Rename toggle --> open

97e484b

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make navigation changes in one go

4f05d9a

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove text(..)

1772fc0

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Change font size

5e4db2f

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove selector list

e161f61

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

bwindels reviewed Jun 24, 2021

View reviewed changes

MidhunSureshR and others added 4 commits June 24, 2021 13:48

Remove Segment import

e2fd90b

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Remove css assumption

a273b25

Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Make sure room does exist before creating vm

eb35f46

- This will stop the code from throwing when opening /details on UnknownRoomView. Signed-off-by: RMidhunSuresh <rmidhunsuresh@gmail.com>

Merge branch 'master' into room-info

09aba78

bwindels approved these changes Jun 24, 2021

View reviewed changes

bwindels merged commit 80fff87 into element-hq:master Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add right panel with Room information #365

Add right panel with Room information #365

MidhunSureshR commented May 23, 2021

bwindels commented May 31, 2021

bwindels left a comment

bwindels left a comment

bwindels left a comment •

edited

bwindels commented Jun 14, 2021

bwindels left a comment

bwindels Jun 15, 2021

bwindels Jun 24, 2021

bwindels Jun 24, 2021

Add right panel with Room information #365

Add right panel with Room information #365

Conversation

MidhunSureshR commented May 23, 2021

bwindels commented May 31, 2021

bwindels left a comment

Choose a reason for hiding this comment

bwindels left a comment

Choose a reason for hiding this comment

bwindels left a comment • edited

Choose a reason for hiding this comment

bwindels commented Jun 14, 2021

bwindels left a comment

Choose a reason for hiding this comment

bwindels Jun 15, 2021

Choose a reason for hiding this comment

bwindels Jun 24, 2021

Choose a reason for hiding this comment

bwindels Jun 24, 2021

Choose a reason for hiding this comment

bwindels left a comment •

edited