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 #726

Merged
merged 11 commits into from
Jul 20, 2018
Merged

feat: peers page #726

merged 11 commits into from
Jul 20, 2018

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented Jul 16, 2018

Tasks:


Closes #700.

@fsdiogo fsdiogo added the revamp label Jul 16, 2018
@fsdiogo fsdiogo self-assigned this Jul 16, 2018
@fsdiogo fsdiogo added the status/in-progress In progress label Jul 16, 2018
@fsdiogo fsdiogo changed the title [WIP] feat: peers page feat: peers page Jul 17, 2018
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

The code looks good, but two notes:

  1. It seems to bit a bit slow. Maybe the list could start with a maximum of 10 peers and then we'd have the option to show more?
  2. I know my country/flag emojis are crap. Could you add a margin right here?

image

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jul 18, 2018

@hacdias Slow when scrolling or fetching the flags? I'm using react-virtualized to only render the visible rows, but you can scroll to see more.

I can add the margin, but it was not supposed to appear US there, only flags 😅

@hacdias
Copy link
Member

hacdias commented Jul 18, 2018

@fsdiogo it seems that Windows Flag emojis are just the country code sadly. Yesterday it seemed more slower. When I resize the page and scroll it seems a bit slow

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jul 19, 2018

@hacdias ohh, so the flags aren't rendering on Windows? 😓

Can you test the following please (I don't have a windows machine):

In the src/peers/PeersTable/PeersTable.js, line 14, can you change that to:

{ rowData.flagCode ? <CountryFlag code={rowData.flagCode} svg /> : '🏳️‍🌈' }

That way we are force rendering the flags to SVG instead of emoji. Maybe it will work.

@hacdias
Copy link
Member

hacdias commented Jul 19, 2018

@fsdiogo

Yes, that way they render:

image

Still, we need some margin there

@hacdias
Copy link
Member

hacdias commented Jul 19, 2018

@fsdiogo merged #702

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jul 19, 2018

@hacdias awesome, thanks!

It doesn't look as good as with the emojis though:

screen shot 2018-07-19 at 11 04 05

We have two options here:

  1. Always render the flags as SVG
  2. Use something like platform.js to detect if it's Windows, and render as SVG accordingly

What do you think?

@hacdias
Copy link
Member

hacdias commented Jul 19, 2018

Yeah, the emojis are better even though they are platform dependent. You can render with SVGs on Windows or let it use the Windows "emojis". If we render SVGs conditionally, I don't think there's a need to add a dependency when we can just:

svg={window.navigator.appVersion.indexOf('Win') !== -1}

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

Despite that comment bellow, the code LGTM and I think it can be merged 😄

return peers && peers.map((peer, idx) => ({
'id': peer.peer.toB58String(),
'address': peer.addr.toString(),
'location': 'New York, United States'
Copy link
Member

Choose a reason for hiding this comment

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

Why's New York here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that selector is not being used, I made a new below. I'll commit a fix.

@@ -0,0 +1,4 @@

.PeersTableContainer {
height: 220px;
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but I'd just use an inline style attributes for this as the value is duplicated in the definition of the table. I'd inline .MapContainer as well, just to keep all the definitions together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, and that way we can delete those two CSS files 👍

Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

This looks great and works well ✨

I've added a minor comment, that I leave to you @fsdiogo to decide on.

In general, we're gonna have to figure our some tricks to make this not spin the fans on everyones machine. I know @hacdias is already looking into it.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jul 20, 2018

Can we merge this? 🚀

@hacdias hacdias merged commit 505fbaa into revamp Jul 20, 2018
@hacdias hacdias deleted the feat/peers-page branch July 20, 2018 11:04
@hacdias
Copy link
Member

hacdias commented Jul 20, 2018

@fsdiogo here we go 😄

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

Successfully merging this pull request may close these issues.

3 participants