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

Cleanup chat/userlist to use flexbox, fix a couple of bugs #2150

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Mar 4, 2018

  • Chat/sidebar are now positioned using flexbox and not absolute positioning
  • Fix toggling userlist not keeping scroll to bottom correctly
  • Fix up/down arrows in search bar changing caret position
  • Fix error when trying to use arrows while search results are empty
  • Changed scrolling code in userlist keybinds to use scrollIntoView(false)
  • Changed .sidebar .users to .userlist to reduce confusion
  • Userlist toggling is no longer animated, this is an intentional change
  • Fixed On iOS, the user list does not seem to be scrollable #1548 - scrolling userlist on iOS devices

@xPaw xPaw added the Meta: Internal This is an internal codebase change (testing, linting, etc.). label Mar 4, 2018
@astorije
Copy link
Member

astorije commented Mar 4, 2018

Possibly fixed scrolling userlist on iOS devices (#1548), needs testing

Yes! I confirm it's fixed with this PR, switching to "Bug" label.

@astorije astorije added this to the 3.0.0 milestone Mar 4, 2018
@astorije astorije added Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. and removed Meta: Internal This is an internal codebase change (testing, linting, etc.). labels Mar 4, 2018
@astorije
Copy link
Member

astorije commented Mar 5, 2018

Userlist toggling is no longer animated, this is an intentional change

Why? I'm not sure I'm in favor of that. Among other things, this means channel list is animated, but user list isn't (channel list animation when clicking the menu button is nice because it mimics the movement the channel list does when swiping it open).

@xPaw
Copy link
Member Author

xPaw commented Mar 5, 2018

Why? I'm not sure I'm in favor of that

I really think the animation just gets in the way here, it doesn't add anything. It also looks somewhat disconnected as the chat itself is not animated (we discussed this before, we don't animate it due to reflowing and keeping the scroll position, and importantly performance).

As for the channel list, the current problem I have with it is that it's implemented in a different way (can't even hide it on desktop!).

I also want to look into making network and user list into "overlays" on mobile view, so that it doesn't push the chat around, and there I can look into animating it.

@xPaw
Copy link
Member Author

xPaw commented Mar 5, 2018

Okay I went ahead and implemented userlist as an overlay on mobile.

  1. On desktop, it pushes chat, there is no animation
  2. On mobile, it's an overlay over chat, it's animated

Can you tell me if you prefer this implementation, and if the userlist scrolling still works on iOS.

@AlMcKinlay
Copy link
Member

AlMcKinlay commented Mar 5, 2018

This looks pretty good, and works quite well.

I think I like the overlay, the chat moving was always a bit odd on mobile.

One thing I've noticed is that on mobile, if the number of users is low, and it doesn't overflow the screen, then the sidebar just stops at the end of the names. Other than that, seems good in my testing.

@xPaw
Copy link
Member Author

xPaw commented Mar 5, 2018

@McInkay fixed.

@astorije
Copy link
Member

astorije commented Mar 6, 2018

Can you tell me if you prefer this implementation, and if the userlist scrolling still works on iOS.

Yes, this is a great compromise (I also agree that overlay on mobile was a must 👏) and I confirm it still works fine on iOS 🎉

Great stuff, and thanks for being open to alternatives :) I'ma review fully now

Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Pretty awesome stuff!

Code is actually rather hard to review because it solves many things and touches many places, but quick test showed it works well.
We have a lot of time before stable v3 to check any actual issues, so 👍

@astorije astorije requested a review from AlMcKinlay March 6, 2018 04:04
Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

Seems to work well. I also haven't looked through every single piece of code in this one, because that would be silly, really.

@xPaw xPaw merged commit 85efebc into master Mar 6, 2018
@xPaw xPaw deleted the xpaw/chat-flexbox branch March 6, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants