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

Remove Pinning and add Add By Path #655

Merged
merged 11 commits into from
Sep 5, 2018
Merged

Remove Pinning and add Add By Path #655

merged 11 commits into from
Sep 5, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Aug 28, 2018

Closes #652

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>
@hacdias hacdias changed the title [wip] Remove Pinning and add Add By Path Remove Pinning and add Add By Path Sep 4, 2018
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.

Works as expected, ok to merge after some minor polish:

  • Dialog window is missing "OK" button

    screenshot_10

  • Add from disk and Add fro Path look the same:

    screenshot_11

    We will redesign this anyway, so maybe for now just add titles that show up on mouse hover.

    • inline comments below

try {
await ipfs().files.stat(PATH)
} catch (_) {
return
Copy link
Member

Choose a reason for hiding this comment

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

Return silently only if error is Error: file does not exist.
Everything else should be logged.

const sendPinning = () => { send('pinning', pinning > 0) }
const inc = () => { pinning++; sendPinning() }
const dec = () => { pinning--; sendPinning() }
await ipfs().files.mkdir('/pinset_from_old_ipfs')
Copy link
Member

Choose a reason for hiding this comment

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

+desktop: pinset_from_old_ipfs_desktop

const { ipfs } = opts

return async (_, hash, path) => {
if (!hash.startsWith('/ipfs')) {
Copy link
Member

Choose a reason for hiding this comment

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

People may try to add /ipns/ paths, and that should not be prefixed.

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>
@hacdias hacdias closed this Sep 4, 2018
@hacdias hacdias reopened this Sep 4, 2018
@hacdias
Copy link
Member Author

hacdias commented Sep 4, 2018

@lidel I changed the icon to the pinning one just to make it more familiar for now. What do you think?

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.

Better, but still need those titles :P


const path = await prompt({
title: 'Add by IPFS Path',
label: 'How do you want to tag it?',
Copy link
Member

Choose a reason for hiding this comment

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

Now that we are in Files "How do you want to name it?" is a better label.


const hash = await prompt({
title: 'Add by IPFS Path',
label: 'Write the IPFS path to add:',
Copy link
Member

Choose a reason for hiding this comment

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

"Enter the IPFS path to add:"

@@ -144,6 +177,7 @@ class Files extends Component {
<IconButton active={this.state.sticky} onClick={this.toggleStickWindow} icon='eye' />

<div className='right'>
<IconButton onClick={this.addFromIPFS} icon='pin-alt' />
Copy link
Member

Choose a reason for hiding this comment

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

IconButton should accept title as one of its props, and use it like this:

<button title="Add by IPFS Path">[icon]</button>

We really need a tooltip that describes what the button does :)

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Sep 4, 2018

Done @lidel 😄

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.

Thanks! I feel this should be enough for now, we don't want to spend too much time on this.
When WebUI lands we will get back to Desktop with proper redesign :) 👍

Before merging this, check logging bug below 🙃

try {
await ipfs().files.stat(PATH)
} catch (e) {
if (e.toString().indexOf('file does not exist') !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant === here :) Avoid indexOf, try using String.includes instead, it is easier to read and harder to make a mistake.

In this particular case, this should be enough:

if (e.message !== 'file does not exist') {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Merging now 😄

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias merged commit bde02e4 into master Sep 5, 2018
@hacdias hacdias deleted the remove-pinning branch September 5, 2018 07:49
@lidel
Copy link
Member

lidel commented Sep 5, 2018

@hacdias would be good to make a new release with this fix.
New users would not need to see migrated pinset when new WebUI lands :)

@hacdias
Copy link
Member Author

hacdias commented Sep 5, 2018

@lidel I'd like to but Jenkins isn't building for Windows and there seems to be an issue with macOS I can't replicate (#656)... Doesn't seem to be a good time for release 😞

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

Successfully merging this pull request may close these issues.

2 participants