-
Notifications
You must be signed in to change notification settings - Fork 490
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: highlight local network peers #1266
Conversation
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
I thought maybe coloring the row with a different background for local and geographically close peers, but that wouldn't say much by itself. Maybe some badge near the location? |
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias are you able to create screenshots with 2-3 options? (eg. yellowish background / text color / badge). Will be easier to evaluate that way. |
Keeping the emoji for the Local Network ones, we could use this colors (gold for local network, silver for nearby geographically): Same, but with light yellow for local network and light blue for nearby geographically: Keeping the emoji for the Local Network ones, using the color scheme gold for local network and blue for nearby geographically: Last but not the least, with badges! In this case, I don't believe keeping the Local Network emoji is worth it because we have badges! What do you like most? I really love the 'Local Network' with a handshake. I'd personally go to either one of the last two. |
Love the handshake, too! Given that this indicator is all about proximity, i think it makes sense to use a small label, as opposed to stretching it across the entire row. It also seems useful to explicitly mark them as "local" or "nearby," so we don't have to rely on a key, eg. In the case of local connections, what if we swapped out the globe for the handshake, and swapped out "Unknown" for "Local"? In theory, the user would know the location of a locally connected peer, right? 😸 For nearby nodes, i've simply added "(nearby)" to the end of the nearby node (in gray) below. Not a brilliant solution, admittedly. We could add a small emoji, perhaps … |
Thanks! I feel changing text/background color makes things too noisy (we may want to keep that option for other things like "manually added peers") For those reasons I'd keep regular colors, handshake icon and full "Local Network" to be explicit what we mean by local: |
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
Ping @lidel @autonome @ericronne |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unqualified to vouch for code, but re the visual and copy changes: ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor questions about exception handling, code locality, etc.
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
@autonome just updated the function with memoization (from a package though) and added comments! |
Based on #1262 so that must be merged beforehand. This PR introduced a special location for local network peers, where show their flag as a handshake with the 'Local Network'. This idea comes from #223 which suggests to highlight local network and close peers geographically.
I wonder if this is the best way to show local network peers. How'd highlight geographically close peers?
/cc @lidel @autonome