Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Enhance site position avatar UI/UX for RFC-001 #332

Merged
merged 17 commits into from
Mar 8, 2018

Conversation

jasonrudolph
Copy link
Contributor

@jasonrudolph jasonrudolph commented Mar 6, 2018

Depends on atom/teletype-client#54

Description of the Change

This pull request updates the site position avatars in support of #268.

#286 explored various designs, and this pull request implements the design described in the last mockup in #286 (comment):

mockup

Verification Process

  • Verify proper rendering of avatars and link icon in popular themes
  • Verify proper rendering of link icon when using small and large font sizes
  • Given a participant that is viewing the same editor that you're viewing, verify that you can click on their avatar to go to their current location in the editor and begin following them
  • Given a participant that is viewing a different editor than the one you're viewing, verify that you can click on their avatar to go to their current location in their current editor and begin following them
  • Given a participant that is viewing a non-portal pane item (e.g., the Atom "Settings" tab), verify that you cannot click on their avatar to follow them
  • When you are following another participant, verify that:
    • Scrolling to a different area of the editor disconnects your tether
    • Switching to another editor disconnects your tether
    • When they scroll to an area of the editor that isn't visible in your viewport, your viewport scrolls to their new position
    • When they switch to another editor in the portal, you follow them to that new editor
    • When they switch focus to a non-portal pane item:
      • You see their avatar's opacity lighten, but you remain at your current position in your current editor
      • When they switch focus to an editor in the portal, you see their avatar's opacity darken, and you resume following them

TODO

Applicable Issues

Closes #268.
Closes #286.

Prior to this change, we rendered three different site position
components (upper right, middle right, and lower right), and a
participant's location in the current file determined which site
position component they appeared in. This seemed like useful behavior
when we were first designing the Teletype UI, but in practice, this
behavior isn't useful enough to justify the complexity.

With the changes in this commit, we now render a single site position
component in the lower right corner of the active text editor, and we
show all site participant avatars in that component.
This is an initial step toward the design shown in
#286 (comment).
.icon-link {
position: absolute;
padding-left: 28px;
padding-top: 28px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simurai: I really dig the what the link icon appears in your mockup:

This pull request currently hacks in a pretty rudimentary implementation:

image

Would you be willing to update the styling to match your mockup? You can feel free to push directly to this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added an extra pseudo element as cover:

teletype

@simurai
Copy link
Contributor

simurai commented Mar 7, 2018

  • Add padding to avoid covering the last few lines

This seems harder than it sounds. I tried adding padding-bottom: 70px to some elements, but most things in the editor are absolute positioned. I think it needs the same mechanic like the "Scroll Past End" setting. That one increases the height of .lines and parent container. So if there isn't an easy solution, I would say we can punt on this for now. Like in case somebody runs into the problem that the avatars cover some code, they can:

  • Just add a few extra lines
  • Resize the window
  • Enable "Scroll Past End"

Not the best experience, but also not unsolvable and there are other higher priorities.

@simurai
Copy link
Contributor

simurai commented Mar 7, 2018

Any thoughts on the gracefulness? Currently participants just get cut off when there is not enough space:

teletype

Maybe already fine? Otherwise we would start optimizing for the 0.1%, if this chart is still accurate.

In the rare case where there are so many participants that we can't fit
all of their avatars horizontally across the bottom of the editor pane
item, we'll simply hide the avatars that overflow. See
#332 (comment).
For now, we're not going to render avatars differently for participants
that are in the  same editor that you're viewing versus participants
that are in a different editor than the one you're viewing.
@jasonrudolph
Copy link
Contributor Author

@simurai: Thanks for these updates! I asked for help with one thing, and you took the time to help with 3 things. 😻

One follow-up request: In my testing, the link icon seems to render well when the using a font size of 14 or smaller. When I increase the font above 14, the link icon's position gets distorted. (See examples below.) Can you push up some changes to fix this issue or offer some tips for how I can go about fixing it?

14-point font

screen shot 2018-03-07 at 09 32 48 am

15-point font

screen shot 2018-03-07 at 09 33 30 am

20-point font

screen shot 2018-03-07 at 09 33 38 am

Prior to this commit, if a site was viewing a non-portal pane item
(e.g., the Atom "Settings" tab), the SitePositionsComponent did not show
the site's avatar. If you know that a given person is in the portal, it
can be confusing to not see their avatar in the SitePositionsComponent.

With the changes in this commit, the SitePositionsComponent will now
show the avatars for *all* sites. If a site is viewing a non-portal pane
item, the SitePositionsComponent gives the avatar a lighter opacity to
distinguish it from the avatars of sites that are currently viewing an
editor associated with the portal.

As part of this change, this commit consolidates SitePositionsController
and SitePositionsComponent into one class: SitePositionsComponent.
@jasonrudolph
Copy link
Contributor Author

I originally included this as something we might do in this pull request:

  • Maybe: Show tooltip when hovering over an avatar (e.g., "Follow @as-cii to foobar.cpp:34")

I still like the idea, but this pull request already involves quite a few changes, and this ☝️ would make sense as a standalone enhancement that we address in a future pull request. So, I've removed this task from the list of TODOs in the pull request body.

@jasonrudolph jasonrudolph requested a review from as-cii March 7, 2018 21:50
@jasonrudolph jasonrudolph changed the title [WIP] Enhance site position avatar UI/UX for RFC-001 Enhance site position avatar UI/UX for RFC-001 Mar 7, 2018
@simurai
Copy link
Contributor

simurai commented Mar 8, 2018

One follow-up request:

Ohh.. right. Good catch. Ok, it's now locked to a fixed font size:

teletype


I still like the idea

I also like that idea. Next to telling what clicking does, it's also a reminder who it is, in case you're not familiar with the avatar.

@jasonrudolph
Copy link
Contributor Author

Good catch. Ok, it's now locked to a fixed font size...

Thanks, @simurai. ⚡🙇💟

@jasonrudolph jasonrudolph merged commit b9268e5 into master Mar 8, 2018
@jasonrudolph jasonrudolph deleted the better-site-position-avatars branch March 8, 2018 16:21
@xmsid
Copy link

xmsid commented Mar 23, 2018

The "Link" icon that appears next to a followed collaborator looks weird on my side
image
I tried different UI/Syntax themes but it still looks like this. is this intended to look like that?

@winstliu
Copy link
Contributor

@xmsid I think what you're seeing was fixed in #351 so it should be in a future Teletype release.

@xmsid
Copy link

xmsid commented Mar 23, 2018

@50Wliu yeah, I checked that issue it's the same as mine, I'm really looking for more future releases :).

@jasonrudolph
Copy link
Contributor Author

I'm really looking for more future releases :).

@xmsid: Awesome. Stay tuned. 😄

@rsanheim
Copy link

Awesome work y'all! ✨

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants