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

Novo Layout: Morpheus #174

Merged
merged 39 commits into from
Jul 18, 2019
Merged

Conversation

megatroom
Copy link
Contributor

"This is your last chance. After this, there is no turning back. You take the blue pill - the story ends, you wake up in your bed and believe whatever you want to believe. You take the red pill - you stay in Wonderland and I show you how deep the rabbit-hole goes."

The red pill

Now that you've entered the Matrix, here's the new layout:

office

I used the concepts of Material Design and is fully responsive.

You can see all online users through the side menu:

office-sidebar

The side menu has a search for you can find a user more quick.

Now the video conference is part of the page, so it is possible to take advantage of the menu for the room's own functions:

call

And you still open the side menu:

call-sidebar

You can use the side menu to invite some user to your current meeting:

invite

The user will receive the invitation to join the meeting.

If you want to find a new meeting room fast, you can click in a menu filter to show only the rooms with people:

active-filter

You can search by the name instead:

search

If new users join a meeting, you will recieve a notification:

notifications

Backward Compatibility

This new layout is only a proposal, although it already has a lot ready, this new version does not affect the current version, can be used together.

By default you will enter the room that exists today, but if you follow the white rabbit you can use this new version.

Follow the White Rabbit...

There are a few things to adjust, both in code and functionality, but if the proposal is interesting to you, I give continuity to the development.

I hope you all liked 😁

@hashtegner
Copy link
Member

Awesome @megatroom!
I will review and test asap.
"There is no way back from here"
https://www.youtube.com/watch?v=fTaOlBWcl48

Copy link
Contributor

@juliemar juliemar left a comment

Choose a reason for hiding this comment

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

Hi @megatroom you are rock \oo/. I'm executing basic tests and I will put my comments.

  1. one of the main features is to show where you are available. I noticed that in this suggestion every time when I left the meeting my user is redirected to the default room. The rooms is a good way to identify team and areas. And sometimes I only want to show that I'm online working with my team. You can see this use case in this print:
    image

@megatroom
Copy link
Contributor Author

Hi @megatroom you are rock \oo/. I'm executing basic tests and I will put my comments.

  1. one of the main features is to show where you are available. I noticed that in this suggestion every time when I left the meeting my user is redirected to the default room. The rooms is a good way to identify team and areas. And sometimes I only want to show that I'm online working with my team. You can see this use case in this print:
    image

I thought it was a bug staying in the room after leaving the Jitsi, I had understood that the fact of being in the room the user would be mandatory in the meeting (Jitsi).

Now it makes more sense, and it makes sense now to have the option to get into meeting in the standard room, and now it made sense of this headset icon.

This was not clear because there is only one button to enter the room, and when you enter it automatically enters the Jitsi. It would make more sense to have two buttons then: "Enter room" and "Enter meeting". What do you think?

@juliemar
Copy link
Contributor

juliemar commented Jul 2, 2019

Hi @megatroom sometimes I need to send the link of my current room to other users, and I miss the room link in this propose:

Current version:
image

Morpheus Version:
image

I'm in room logos. In this case, is not possible to send the room link for other users to enter directly in my current room.

@juliemar
Copy link
Contributor

juliemar commented Jul 2, 2019

This was not clear because there is only one button to enter the room, and when you enter it automatically enters the Jitsi. It would make more sense to have two buttons then: "Enter room" and "Enter meeting". What do you think?

I agree with you, the current UX is not good. by the way, is so important to pin my user in room of my team for example. This feature enables other people to quickly identify the team and who is working with this treams.

@megatroom
Copy link
Contributor Author

Hi @megatroom sometimes I need to send the link of my current room to other users, and I miss the room link in this propose:
I'm in room logos. In this case, is not possible to send the room link for other users to enter directly in my current room.

It makes perfect sense. I'll implement that.

I agree with you, the current UX is not good. by the way, is so important to pin my user in room of my team for example. This feature enables other people to quickly identify the team and who is working with this treams.

What do you mean by pin the user? Do you wanna group the users in the sidebar by room, or do you want to mark the user as a favorite or put a tag so it goes to the top of the list?

@juliemar
Copy link
Contributor

juliemar commented Jul 3, 2019

What do you mean by pin the user? Do you wanna group the users in the sidebar by room, or do you want to mark the user as a favorite or put a tag so it goes to the top of the list?

I'm sorry about my mistake. Is important to enter in the room and stay there. Is not necessary to be in the call to enter the room. the room concept is to inform where you are available. because this is important to be in a room even without being on a call.

@megatroom
Copy link
Contributor Author

I made a video: https://drive.google.com/file/d/1F2BLQfJstNQQCpzSS4LejH9QxkYrMNWV/view

my expectation is: when I enter the room the URL changes like when I enter in call.. maybe we can have two routes.

  1. room meet link as you did (it was very good)
  2. room link like I'm trying to show in my video

What do you think?

Now I got it, you wanna in the main screen, where it has the room grid, the URL is with the id of the room that you are.
Okay, I'm going to implement this route.

@megatroom
Copy link
Contributor Author

@juliemar , I've finished all changes.

Let me know if everything is ok.

@juliemar
Copy link
Contributor

juliemar commented Jul 9, 2019

  1. When you are not logged and try to access the morpheus URL the we have a redirect loop.
    video: https://drive.google.com/file/d/11oALrio7Xyp_itIucHX4SJeZ_w_0Vj33/view

  2. Web notifications: In this propose the web notifications notify all other users room change. We tried this in the first version and had to change because the users don't know the other user's movement. They only want to know if a user enters or left their current room. And I missed the web notification in desktop. is important to be desktop because some times I'm using other software or have an office minimized and have to know if someone is calling or entered in my current room.
    video: https://drive.google.com/file/d/1VzmzErr_wNrpp7hdF7RjXtyJlwRUnbju/view

this is the commits that I mentioned in my video.
d8dd46c
2345dbd

@megatroom
Copy link
Contributor Author

  1. When you are not logged and try to access the morpheus URL the we have a redirect loop.
    video: https://drive.google.com/file/d/11oALrio7Xyp_itIucHX4SJeZ_w_0Vj33/view
  • I resolved the infinite loop.
  • I was relying on the localStorage to set whether the user was logged in, I changed this and placed to validate the login by the Google API. In addition to getting more consistent, I reload the localStorage if it is empty.
  1. Web notifications: In this propose the web notifications notify all other users room change. We tried this in the first version and had to change because the users don't know the other user's movement. They only want to know if a user enters or left their current room. And I missed the web notification in desktop. is important to be desktop because some times I'm using other software or have an office minimized and have to know if someone is calling or entered in my current room.
  • I changed to only display the notification for when a user enters the same room as you are.
  • I added a button in the menu to disable the notification in case you do not want to receive them temporarily.
  • A browser's notification was already being sent in this case, what may be happening is that you may not have allowed the notification. To make it easier to identify this problem, if the notification is blocked or if the user has not yet made this decision, the notification icon I added will get an exclamation mark, and clicking on it will attempt to enable or be warned that the user has blocked the notification.

@juliemar
Copy link
Contributor

@Alesshh and @lccezinha for me is everything OK. I only reviewed the functionalities, about code I will let for you :)

@juliemar juliemar requested review from juliemar and removed request for raphaelln July 10, 2019 18:40
Copy link
Contributor

@juliemar juliemar left a comment

Choose a reason for hiding this comment

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

For me, we have a good version to have an experiment in production. Other improvements we can implement incrementally.

Copy link
Member

@hashtegner hashtegner left a comment

Choose a reason for hiding this comment

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

Hey @megatroom , sorry for the delay in review.
I added some comments, if imakes sense.
I has 2 concerns:

  • I think we can be restrict with props, using optional / default props only where is necessary, leaving props less verbose
  • Use of mocha and jest: I don't have personal preferences, but it would be nice if we can live only with one test framework, reducing complexicity / dependencies. We can kill jest depencendy after?

Other think is about the approach of structure by function instead feature (https://medium.com/better-programming/scaling-your-redux-app-with-ducks-6115955638be). I like the approach by feature, because I can easy locate and isolate things by context, and easy identify shared components. Mainly in medium/large apps this structure tends to get confused, but for now, we can live with this.

!(user.inMeet && user.roomId === currentRoom.id);

return (
<ListItem key={user.id}>
Copy link
Member

Choose a reason for hiding this comment

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

What about split into a MenuUsersItem component (or similar) to separete responsibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it turned out that this component was quite large. I did as you suggested.

</Tooltip>
);
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can separate into small components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NotificationCheckbox is already a subset of MenuOffice component to concentrate the logic of menu notification button. And the "@material-ui" already encapsulates the presentation layer of the components, you may notice that there is no style here, just behavior. So my fear is that with this division it makes maintenance difficult without having any gain in return. What do you think?

gapi.load("auth2", () => {
gapi.auth2.init().then(
auth2 => {
if (auth2.isSignedIn.get()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think early returns is better in if else statements.

if (!auth2.isSignedIn.get()) {
    return window.location.href = "/";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to early return, but I can't put the return on the same line because it's an assignment, the lint complains about it.

frontend/morpheus/App.js Outdated Show resolved Hide resolved
frontend/morpheus/App.js Outdated Show resolved Hide resolved
if (!settings.notificationDisabled) {
showBrowserNotification(
`${user.name} is inviting you to ${room.name}`
);
Copy link
Member

Choose a reason for hiding this comment

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

Tell showBrowserNotification, don't ask about settings.notificationDisabled where, same case about composition.

} else {
setLoading(false);
setMeetingDialogOpen(true);
}
Copy link
Member

Choose a reason for hiding this comment

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

Early returns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't use early return inside the hook useEffect because the return of your callback is used for its cleanup.

}
};
case ADD_USER: {
const index = state.usersInRoom.findIndex(
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is use a map instead array to store users per room. It's reduce complexity from o(n) to o(1) and simplify add / remove users.

const usersInRoom = {}
usersInRoom[action.user.id] = { user: action.user, room: roomId }  } 
Object.values(usersInRoom) // [{user: ...}, {user: ...}]
...


export const selectError = state => state.error;

export const selectSettings = state => state.settings;
Copy link
Member

Choose a reason for hiding this comment

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

I think this file generate an unnecessary dependency and complexity.
My suggestion is:

  • use mapStateToProps only where necessary
  • rely in default props with explicit required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand, could you explain?

@megatroom
Copy link
Contributor Author

Hey @megatroom , sorry for the delay in review.
Hello @Alesshh ! Thanks for the review.

I added some comments, if imakes sense.
I has 2 concerns:

  • I think we can be restrict with props, using optional / default props only where is necessary, leaving props less verbose

When it's too restrictive to output contracts in front-end it ends up reducing the application resilience. The default values are a guarantee for the component to be rendered even in the absence of any value. It's a part of defensive programming.

To improve props validation, I came back with ESLint's react/forbid-prop-types rule and I specified each array and object better.

What do you think?

  • Use of mocha and jest: I don't have personal preferences, but it would be nice if we can live only with one test framework, reducing complexicity / dependencies. We can kill jest depencendy after?

I removed the Jest and left only Mocha testing everything.

I really like Mocha, it's even my first choice for back-end testing. However, I like Jest to test front-end, mainly projects with React, since it already comes ready with jsdom, and come with assertion library and mock by default.

Apart from personal preference for Jest, I also find it a little strange to have the back-end and front-end tests all together in the same directory. Besides it's looks like a unnecessary coupling, it's difficult to configure, as for example I had to repeat the ESLint configuration.

Other think is about the approach of structure by function instead feature (https://medium.com/better-programming/scaling-your-redux-app-with-ducks-6115955638be). I like the approach by feature, because I can easy locate and isolate things by context, and easy identify shared components. Mainly in medium/large apps this structure tends to get confused, but for now, we can live with this.

I like modularization by functionality, and also Redux Ducks, this was for a long time my preferred form of organization, but currently I have followed this pattern of organization for any project.

If you look at the history of commits I made, you will see that I started using React only with useReducer in App.js to control the global state, with the files and component all in the same directory as I was evolving, I was organizing and adding more things, the Redux I put practically at the end, when I had already opened the PR.

We can organize the files on demand in the next PR's.

Copy link
Member

@hashtegner hashtegner left a comment

Choose a reason for hiding this comment

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

Let's merge the pr and move on, today let's put into production here to see how it comes out

@hashtegner hashtegner merged commit 7e36d1d into ResultadosDigitais:master Jul 18, 2019
@juliemar juliemar mentioned this pull request Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants