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.

Sign up for GitHub

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

Jump to bottom

feat: add connection #969

Merged
merged 7 commits into from
Mar 19, 2019
Merged

feat: add connection #969

merged 7 commits into from
Mar 19, 2019

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Feb 22, 2019 •
edited
Loading

Closes #891.

As a first iteration this is what I've done:

image

image

It works, but we don't provide feedback if it worked or not. What do you think?

/cc @lidel @olizilla @fsdiogo

@hacdias
feat: add connection
8af80fd 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
feat: translate one sentence
f6800ad 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@ghost ghost assigned hacdias Feb 22, 2019
@ghost ghost added the status/in-progress In progress label Feb 22, 2019
@lidel
Copy link
Member

lidel commented Feb 22, 2019 •
edited
Loading

Cool! Quick feedback: we should provide an Examples as a hint to the user that address should be in multiaddr format, similar to what we already do on Files → Add from Path:

2019-02-22--15-36-50

fsdiogo
fsdiogo reviewed Feb 22, 2019
View reviewed changes
.vscode/settings.json Outdated Show resolved Hide resolved
@fsdiogo
Copy link
Contributor

fsdiogo commented Feb 22, 2019

It works, but we don't provide feedback if it worked or not.

We should provide feedback! Why not use the notify bundle as:

notify

A message saying Successfully connected to the swarm or something like that is sufficient IMO!

@hacdias
fix: remove vscode stuff
a370a04 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
feat: notify on error/success
0bc4561 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Feb 23, 2019

image

image

image

@hacdias
fix: revert pkg-lock
9cca512 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias changed the title [wip] feat: add connection feat: add connection Feb 23, 2019
@hacdias
Copy link
Member Author

hacdias commented Feb 23, 2019

I think it's ready for review!

lidel
lidel requested changes Feb 23, 2019
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Could not connect to the swarm
Insert the swarm address you want to connect to.

I believe it makes sense to replace "swarm" with "peer" in all labels/messages related to "Add Connection".

public/locales/en/notify.json Outdated Show resolved Hide resolved
@lidel lidel mentioned this pull request Feb 23, 2019
Closed
@hacdias
feat: replace by peer
5a8fe90 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@lidel lidel self-requested a review February 24, 2019 01:19
@fsdiogo
Copy link
Contributor

fsdiogo commented Feb 25, 2019

The multiaddr validation in the modal input isn't totally safe, as you can pass an incorrect CID and it will still be accepted, although it will fail to connect.

E.g.: /ip4/192.168.2.92/tcp/4001/ipfs/foobar

@hacdias
Copy link
Member Author

hacdias commented Feb 25, 2019

@fsdiogo yeah we can improve that later with ipfs-shipyard/is-ipfs#26 perhaps.

@hacdias
Copy link
Member Author

hacdias commented Feb 25, 2019

@olizilla can we merge?

@olizilla olizilla mentioned this pull request Feb 26, 2019
Closed
10 tasks
lidel
lidel requested changes Mar 3, 2019
View reviewed changes
src/peers/AddConnection/AddConnection.js Outdated Show resolved Hide resolved
@hacdias
feat: use is-ipfs 0.6
9acbceb 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias requested a review from lidel March 4, 2019 08:51
@lidel lidel requested review from alanshaw and cwaring March 4, 2019 11:51
lidel
lidel approved these changes Mar 4, 2019
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like a native speaker to review proposed labels.

(cc @alanshaw @cwaring)

@olizilla
Copy link
Member

I'd like to keep this as a feature for the 2.5 release, while we are still ironing the kinks out of the analytics stuff for 2.4, that's why i've not reviewed it yet.

olizilla
olizilla approved these changes Mar 19, 2019
View reviewed changes
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.

Super neat ✨! Thank you @hacdias

@olizilla olizilla merged commit fd8f1fa into master Mar 19, 2019
@ghost ghost removed the status/in-progress In progress label Mar 19, 2019
@olizilla olizilla deleted the feat/add-connection branch March 19, 2019 09:55
@fsdiogo fsdiogo mentioned this pull request May 28, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olizilla olizilla olizilla approved these changes

@lidel lidel lidel approved these changes

@fsdiogo fsdiogo fsdiogo approved these changes

@alanshaw alanshaw Awaiting requested review from alanshaw

@cwaring cwaring Awaiting requested review from cwaring

Assignees

@hacdias hacdias

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hacdias @lidel @fsdiogo @olizilla