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

feat: Peers page table enhancements #1602

Closed
6 of 9 tasks
jessicaschilling opened this issue Aug 26, 2020 · 25 comments · Fixed by #1616
Closed
6 of 9 tasks

feat: Peers page table enhancements #1602

jessicaschilling opened this issue Aug 26, 2020 · 25 comments · Fixed by #1616
Assignees
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked topic/design-front-end Front-end implementation of UX/UI work topic/design-visual Visual design ONLY, not part of a larger UX effort
Milestone

Comments

@jessicaschilling
Copy link
Contributor

jessicaschilling commented Aug 26, 2020

Note: This issue includes and therefore supersedes #1037 and #1117.

Summary

Enhance Peers page by adding table columns for transfer rates and IPFS version, and removing unused "Notes" column.

To do

  • Remove "Notes" column from Peers page table
  • Add sortable "In/Out" column between "Latency" and "Peer ID" columns, formatted per mockup:
    • Display TotalIn and TotalOut, or -- if sum(TotalIn, TotalOut) = 0
    • Sort by sum(TotalIn, TotalOut)
    • For units, express as the largest unit that can be used while keeping the value above 1 (i.e. let's avoid things like 0.2 kB in favor of 211 B)
    • If a transfer is currently happening (e.g. if sum(RateIn, RateOut) > 0 ), add an "indicator light" whose colors are the same as the in/out colors on Status page; for edge case in which out/in values are the same, split the circle in two as per mockup
    • Include browser-native hover state indicating current RateIn and RateOut as well as TotalIn and TotalOut, per mockup
  • For "Peer ID" column, make peer IDs clickable, with an action of copying that peer ID to clipboard and feedback using green snackbar: "Peer ID copied to clipboard."
  • For "Connection" column, make row content clickable, with an action of copying that multiaddr to clipboard and feedback using green snackbar: "Connection address copied to clipboard."
  • For "Connection" column, also append direction to the hover text as such: /multi/addr (outbound) (note this doesn't appear if using js-ipfs)
  • Add sortable "Agent" column at the far right, formatted per mockup, using AgentVersion via ipfs id <PeerId>
    • Include hover state indicating what type of activity occurred, i.e. DHT, Bitswap (note this doesn't appear if using js-ipfs)
  • Make items in "Peer ID" column links that open the Explore page with that particular IPNS address
    (peers dont publish anything under their PeerID by default, this would be broken for most of peers)
  • Replace monospace font throughout table with standard Inter for everything except the values in the "Peer ID" column

Mockup screenshot

peers-page-transfer-status

@jessicaschilling jessicaschilling added exp/intermediate Prior experience is likely helpful effort/hours Estimated to take one or several hours kind/enhancement A net-new feature or improvement to an existing feature need/analysis Needs further analysis before proceeding P1 High: Likely tackled by core team if no one steps up topic/design-front-end Front-end implementation of UX/UI work topic/design-visual Visual design ONLY, not part of a larger UX effort labels Aug 26, 2020
@rafaelramalho19
Copy link
Contributor

Transfer column: We risk having multiple values we can sort on here. I assume the highest-impact one, and therefore the sort trigger, is "transfer in" -- thoughts on this?

What about separating it between in & out into their own columns? Would be better for sorting

@jessicaschilling
Copy link
Contributor Author

@rafaelramalho19 Wondered that too, but ended up settling that breaking them into two columns might be disconnecting them too much, and giving each too much importance. If the primary question we're trying to answer is "which peers am I trading with", a single column would be better at indicating that (particularly with the addition of the orange/teal indicator light). Curious what @lidel thinks though.

@lidel
Copy link
Member

lidel commented Aug 26, 2020

Some answers:

  • Transfer column:
    • keep a single column: agree with @jessicaschilling on "which peers am I trading with" being the key utility here, and we want to avoid clutter.
    • sorting: if we sort by sum(in+out) then we always promote peer that generates the biggest traffic – should be enough
    • B/s and kB/s: afaik API will return value in bytes, we can format it any way we want (I'd say a rounded, human-readable values are desired here).
    • always showing values in hover state: sgtm
    • fonts: no strong opinion here, the only thing that has to be monospaced are Peer IDs, everything else can be changed if it makes things look and read better

nits/ideas:

  • Would use of icon/emoji instead of text "in"/"out" help? I worry that translation of these words may be pretty long in some languages.
  • Can we move version to the very right? It is less important than Connection type.
  • Thoughts on making "Version" less ambiguous and rename it to "Agent" (as in "user agent")?
    This will create continuity with ipfs idAgentVersion and decrease vagueness (in IPFS we have many protocols, each has "version", but there is only one "Agent"). Feel free to ignore if I am overthinking this or "Agent" is not the best word for some reason?

@jessicaschilling
Copy link
Contributor Author

@lidel Thanks! I'll update mockup in the morning but one question in the meantime:

Would use of icon/emoji instead of text "in"/"out" help? I worry that translation of these words may be pretty long in some languages

We could, but I don't believe we would be able to include those icons/emoji in the hover state, and that would introduce inconsistency. Thoughts?

@jessicaschilling
Copy link
Contributor Author

@lidel and @rafaelramalho19 -- updated #1602 (comment) to include notes mentioned above. This includes a rough first pass at out/in icons based on our existing stroke_link_external.

Wondering if including an "indicator light" adds noise rather than value -- #1602 (comment) now includes two screenshots, one with lights, one without.

Further thoughts?

@lidel
Copy link
Member

lidel commented Aug 27, 2020

On decreasing visual noise

I like both, unsure how many active transfers regular desktop user would see (more on that later).

In the context of the other visuals, the entire screen got a bit noisy, but I don't think the main offender being the "indicator light". If anything, I think in/out icons are more distracting.

Some ideas (but not feeling too strong about them):

  • Rename column to "Transfer In / Out" or just "In / Out" to act as a legend for values in the column
  • Remove mentioned icons, just values and dot separator (4 kB • 0 B)
  • Display both RateIn/Out and TotalIn/Out in the hover, for additional clarity

Sidethought: should we flip it and show totals instead?

Tweaking noise vs signal ration made me think about how often user will look at Peers screen while there is an ongoing transfer on that exact moment. Desktop users won't have that many data, so unless they happen to download something big at the same time, they will just see boring empty column for the most of the time:

2020-08-28--00-30-55

What if we optimize for the most common, boring state when "nothing happens", and:

  • show totals (if > 0, -- otherwise)
  • sort by sum(TotalIn, TotalOut)
  • display "indicator light" when sum(RateIn, RateOut) > 0 (and then current Rates and Totals on hover)

(Just an idea, I'm ok with us implementing rates as the default first, and then re-evaluating after looking how it behaves irl)

@jessicaschilling
Copy link
Contributor Author

@lidel these are excellent ideas! Adjusted mockup (note German just for idea of longer words) and specifications accordingly.

I'm not 100% convinced that total in/out is more valuable than current in/out, but if we keep the indicator light, that allows us to convey enough info (is a transfer happening right now?) all at once.

I think we might be ready to go here ... any other thoughts?

@lidel
Copy link
Member

lidel commented Aug 28, 2020

Thank you @jessicaschilling, looks good to me, ready for impl.

(I agree it's not ideal, but a good starting point. Just like with the map, where we tweaked clustering so regular users with 100-300 peers have a nice view, we may revisit various aspects after we see how it behaves in real situations.)

@lidel lidel added status/ready Ready to be worked and removed need/analysis Needs further analysis before proceeding labels Aug 28, 2020
@jessicaschilling
Copy link
Contributor Author

Thank you! @rafaelramalho19, do you still have time to take this on? Anything I can help with?

@rafaelramalho19
Copy link
Contributor

Yes, after the breadcrumbs work I'll pick up this 😄

@jessicaschilling jessicaschilling added this to the v2.11 milestone Aug 28, 2020
@lidel
Copy link
Member

lidel commented Sep 7, 2020

As this work did not start yet, I propose we move it to v2.12.
This will enable us to ship v2.11 sooner and be ready for upcoming ipfs-desktop with go-ipfs 0.7.0

Y/n?

@jessicaschilling
Copy link
Contributor Author

OK.

@jessicaschilling
Copy link
Contributor Author

Note - #1616 stubs everything out and meets visual spec, but still needs wiring up.

@andrasfuchs
Copy link

It's great to see the progress, looks great!

Would you consider changing the values in the Peer ID column to links that open the Explore page with that particular IPNS address?

@jessicaschilling
Copy link
Contributor Author

@andrasfuchs -- nice thought!
@lidel, do you see any reason not to?

@lidel
Copy link
Member

lidel commented Sep 18, 2020

@andrasfuchs @jessicaschilling
Main concern: most addresses will never resolve because most of the peers never published anything under the self key, which means user clicks on a link the Explore page tries to resolve /ipns/<peerid> and displays spinner forever, or timeouts after a minute. And that's pretty bad user experience.

What I would do instead, is to copy value clipboard when user clicks on table cell (with some visual feedback).
It could be enabled for all columns, but especially for Peer ID and Connection.

@andrasfuchs
Copy link

@lidel Good idea!

Is it technically difficult/expensice to detect if someone is sharing anything publicly?
If it's cheap(er) then the value in the Peer ID column could be clickable only if there is actually something to see there.

@jessicaschilling
Copy link
Contributor Author

@lidel What would be copied in the Connection column?

Note: Added an additional to-do item to #1616 for feedback upon copy (think we can just use the snackbar for this).

@lidel
Copy link
Member

lidel commented Sep 21, 2020

@jessicaschilling the original Multiaddr value we currently show onhover in title (/ip4/.../p2p/Qm...)

@jessicaschilling
Copy link
Contributor Author

@lidel Thanks for the clarification - added that as a to-do item in #1616.

@lidel
Copy link
Member

lidel commented Oct 19, 2020

Identified two more things that go-ipfs provides and what we could surface on the Peers screen:

  • "direction" – was the connection initiated by my node, or did someone connected to me?
  • "streams" – what type of activity occured? (DHT? bitswap?)
$ ipfs swarm peers --streams --direction
/ip4/A.B.C.D/udp/4001/quic/p2p/QmPeerId outbound
  /ipfs/bitswap/1.2.0
  /ipfs/kad/1.0.0
...

I think those are in the same category as "agent version".
Perhaps we could show them alongside under "agent details" somehow.
This could be a part of #1616 or separate PR in the future.

@jessicaschilling thoughts?

@jessicaschilling
Copy link
Contributor Author

jessicaschilling commented Oct 19, 2020

@lidel Would "streams" activity type make sense as a hover state for a value in the Agent column? We could add that to requirements for #1616.

I'm struggling a bit with where we'd want to display who initiated the connection, particularly since "direction" as a term is easily confused with the data in the in/out column. Do you have any thoughts on how we might be able to word this more clearly in the hover state for the Agent column?

@lidel
Copy link
Member

lidel commented Oct 21, 2020

  • yes, streams could be onhover in Agent column for now
  • similarly, direction could be appended to the onhover text in Connection column (/multi/addr (outbound))
  • sidenote: js-ipfs does not support streams/direction, UI should work fine when API does not return them (we don't want to break webui in js-ipfs)

@jessicaschilling
Copy link
Contributor Author

@lidel SGTM. Does that mean you're going to implement the rest of #1616? ;)

@jessicaschilling
Copy link
Contributor Author

@lidel Requirements have been updated both in the first comment of this issue, and in PR #1616. Anything else I can get you to make this easier?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked topic/design-front-end Front-end implementation of UX/UI work topic/design-visual Visual design ONLY, not part of a larger UX effort
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants