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

Open new Room #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Open new Room #14

wants to merge 3 commits into from

Conversation

KenavR
Copy link
Contributor

@KenavR KenavR commented Jun 6, 2016

Nobody asked for it but since a lot of people seem to ask how to open a room I added a small "join room" icon next to the "Your rooms" headline. I am not that experienced with React, therefore it seems weird to me to implement the CSS :hover in JS, but SO seems to suggest that that is the right way to do it, without using a React-CSS library or adding it to the "global" CSS file.

Visuals

Closed

join_channel_closed

Opened

join_channel_opened

Maybe the font is a little bit big, I increased it to 1.1em, I guess the default would be better suited for the headline, but then it is quite hard to find.

@megamattron
Copy link
Member

@rickhanlonii do you want to take a look at this? Otherwise I'll try to review, FWIW.

this.setState({ value: event.target.value });
}

handlePressedEnter(event) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should trigger chat-actions.handleChangeRoom here instead of doing it manually to make sure we're changing rooms all in one play. Also handleChangeRoom does some other things as well like update the lastMessageTime for the room you change to.

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 tried that, but handleChangeRoomdidn't work with rooms that weren't in the state. I could've changed chat-actions.handleChangeRoom but I thought opening a room over the UI would be the same as using the url. This is also the way I open the room in the 'active rooms' #23 PR. I will take a look at it and update with the concrete issue.

@rickhanlonii
Copy link
Collaborator

Thanks @KenavR! If you make a small update from the one comment I made then this is good to go.

Sorry about the delay!

@megamattron
Copy link
Member

Where are we with this one? I'd love to merge it in since people keep asking for it. Looks like @rickhanlonii was asking for one more change, but @KenavR didn't think it worked? Maybe we can merge it in and you make the change yourself @rickhanlonii ?

@KenavR
Copy link
Contributor Author

KenavR commented Jun 18, 2016

@rickhanlonii @megamattron The issue is that handleChangeRoom tries to get the lastMessageTime from the state before changing the room. Since the room isn't part of the state (first join) it can't find the time. I could now change handleChangeRoom to work with new channels or join them using the url (that's how it is now) since that's the way a user joins a room anyway.

https://github.com/larvalabs/breaker/blob/master/web/redux/actions/chat-actions.js#L65-66

@rickhanlonii
Copy link
Collaborator

@KenavR good point. I would add a handleChangeNewRoom action then. Basically we don't want to be making any side effects inside react components, and moving that logic to an action will allow us to reuse it elsewhere.

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