feat: pinning services stage 1 #1685

rafaelramalho19 · 2020-11-04T18:02:22Z

This is a pull request that marks the stage 1 of pinning services.

This includes:

Local pinning now works with future-proof modal for remote pins
Settings page now reflects how much space local pinning occupies.

Closes #1500
Closes #1501

jessicaschilling

Thank you! 😊 Functionally this seems perfect on my end. Left some comments inline on visual/UX stuff, but a few other thoughts:

On the Settings screen, the left-most table items (both headers and row cells) have both a 10px margin-left and a pl2 on them. This has the effect of the leftmost column of the table looking too indented as a whole. Not sure which styling should be removed to mitigate this, though?
On the Settings screen table, can the rows' cells truncate with ellipses at narrow widths (like the headers do), and can their full values display in hover text (also like the headers)?
We'll need to add explanatory content to the CLI tutor modal to this effect: This command will only pin this item's CID to your local node. If you want to persist it with a remote pinning service, consult that service's CLI documentation to learn how. Right now we have i18n key cliModal.extraNotes for that purpose, but it's effectively hard-coded for one use (the config in Settings). The best way to remedy might be to move cliModal.extraNotes out of app.json and into settings.json and files.json?

public/locales/en/settings.json

src/files/header/Header.js

src/components/pinning-manager/PinningManager.js

src/components/modal/Modal.js

src/files/modals/pinning-modal/PinningModal.js

public/locales/en/files.json

rafaelramalho19 · 2020-11-05T18:02:32Z

We'll need to add explanatory content to the CLI tutor modal to this effect: This command will only pin this item's CID to your local node. If you want to persist it with a remote pinning service, consult that service's CLI documentation to learn how. Right now we have i18n key cliModal.extraNotes for that purpose, but it's effectively hard-coded for one use (the config in Settings). The best way to remedy might be to move cliModal.extraNotes out of app.json and into settings.json and files.json?

I think that's a good point, but it's something that doesn't need to be addressed in this stage :)

jessicaschilling · 2020-11-05T20:13:00Z

@rafaelramalho19 Added #1686 for the CLI tutor mode change - I'll do that real quick once this PR gets merged.

jessicaschilling

LGTM, just one thing that got left out from the original review 😊

On the Settings screen table, can the rows' cells truncate with ellipses at narrow widths (like the headers do), and can their full values display in hover text (also like the headers)?

src/files/header/Header.js

jessicaschilling

On the Settings screen table, can the rows' cells truncate with ellipses at narrow widths (like the headers do), and can their full values display in hover text (also like the headers)?

Otherwise this happens:

lidel

Wrote down some suggestions and one minor blocker inline, but hopefully all those are easy to fix.
Apart from that, looks wonderful! 👍

ps. Some users may be impacted by the fact this PR removes old #/pins screen, and with that, the ability to remove a pin after an item is removed from Files. I don't think this is a problem as long we land ability to Delete and Unpin (#1644). Just mentioning here, so we don't forget.

ps2. Does this supersede #1615? Should I close that PR?

public/locales/en/files.json

src/components/pinning-manager/PinningManager.js

public/locales/en/settings.json

src/files/header/Header.js

src/components/pinning-manager/PinningManager.js

rafaelramalho19 · 2020-11-09T15:22:39Z

Wrote down some suggestions and one minor blocker inline, but hopefully all those are easy to fix.
Apart from that, looks wonderful! 👍

ps. Some users may be impacted by the fact this PR removes old #/pins screen, and with that, the ability to remove a pin after an item is removed from Files. I don't think this is a problem as long we land ability to Delete and Unpin (#1644). Just mentioning here, so we don't forget.

ps2. Does this supersede #1615? Should I close that PR?

Good call, closing it 👍

public/locales/en/files.json

lidel

@rafaelramalho19 seems e2e tests started failing after you merged updates from master – let me know if you are looking into it or got blocked and need a second pair of eyes.

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

* feat: add pinning services service modal * chore: update translation for service modal

* chore: refactor files page * feat: add pinning services mock to files page * chore: update remotePin dimensions Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: update local pin icon dimension Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: change pin icon fill color Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: change remote pin icon fill color Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: update pinning modal text size Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: update pinning modal text size Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: pinning modal secondary text changes Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: update pinning modal image size Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: update pinning modal pin icon Co-authored-by: Jessica Schilling <jessica@protocol.ai> * chore: add pin status column to fileslist * chore: fix modals icons * chore: fix linting error * chore: fix files sorting Co-authored-by: Jessica Schilling <jessica@protocol.ai>

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

lidel · 2020-11-11T00:57:45Z

@rafaelramalho19 I was able to reproduce the CI error locally on Linux with:

npm ci && npm run build && npm run test:e2e

Note that E2E test suite requires a prebuilt static version.
Maybe you were running against older build? Mind checking?

What is interesting is that the tests do not fail when you run debug mode (debug runs tests in slowed down mode and with browser window visible):

DEBUG=true npm run test:e2e

If you disable sloMo then in GUI can see a problem when JSON config is quickly typed into the field – sometimes it becomes garbled:

If I set sloMo to 50 then headless tests are green.
Looks like some kind of regression, not sure what caused it.

Unfortunately I've run out of time to look deeper into this and I'm OOO on Wednesday – will pick up on Thursday if still needed.

rafaelramalho19 · 2020-11-12T12:44:40Z

@rafaelramalho19 I was able to reproduce the CI error locally on Linux with:
npm ci && npm run build && npm run test:e2e
Note that E2E test suite requires a prebuilt static version.
Maybe you were running against older build? Mind checking?

What is interesting is that the tests do not fail when you run debug mode (debug runs tests in slowed down mode and with browser window visible):
DEBUG=true npm run test:e2e
If you disable sloMo then in GUI can see a problem when JSON config is quickly typed into the field – sometimes it becomes garbled:

If I set sloMo to 50 then headless tests are green.
Looks like some kind of regression, not sure what caused it.

Unfortunately I've run out of time to look deeper into this and I'm OOO on Wednesday – will pick up on Thursday if still needed.

Weirdly it doesn't happen in master, so I guess it was due to changes in the settings page in this branch 🤔

lidel · 2020-11-13T13:57:50Z

PSA: I am helping with ipfs/kubo#7661 won't be able to look into this until next week.

This changes the way we enter API address/config from programmatic to full simulation of user input and adds tiny slow down between each key stroke. This should solve the problem of newly added UI feedback not engaging, and make CI both more reliable and green again. While at it, made it CI-agnostic, in preparation for move to GithubActions

lidel

@rafaelramalho19 go-ipfs 0.8.0-rc1 shipped so I was able to switch and fix E2E tests here – details in 7089d93 (better late than never 😅)

Let's discuss next steps during Monday's issue triage,
but if you start work before the call, my quick suggestion is to focus on steps 1 & 2:

Merge this PR – LGTM! ❤️
- nit: calculating total size of all pins is expensive: I have over 1k of pins and it takes minutes of "Loading..." so its pretty useless for users with a lot of pins. That being said, it is not a blocker, it runs in background, and we can revisit/optimize in (2)
Start a new PR to wire up Pinning Services
- while you were OoO go-ipfs 0.8.0-rc1 shipped and Irakli wired things up in feat: support remote pinning services in ipfs-http-client js-ipfs#3293
  - use ipfs-http-client from that PR for now
  - for quick reference, API docs for ipfs.pin.remote commands: https://github.com/ipfs/js-ipfs/blob/feat/pin-remote/docs/core-api/PIN.md
(TBD) tag a release without (2)
- We should tag an intermediate release to be included in go-ipfs 0.8.0 – open a PR and use version from this PR, and if time allows then switch 0.8.0 to version with Pinning Services, if not, we would ship go-ipfs without it and add it in January in 0.8.1
- As this PR removes screen with low level pins, we kinda need to rebase and finish feat: delete-modal pin guidance, improved language #1644 to give users means of removing pins while they remove files from

rafaelramalho19 requested review from lidel and jessicaschilling November 4, 2020 18:02

rafaelramalho19 self-assigned this Nov 4, 2020

rafaelramalho19 added the kind/enhancement A net-new feature or improvement to an existing feature label Nov 4, 2020

jessicaschilling suggested changes Nov 4, 2020

View reviewed changes

rafaelramalho19 requested a review from jessicaschilling November 5, 2020 18:00

jessicaschilling suggested changes Nov 5, 2020

View reviewed changes

src/files/header/Header.js Outdated Show resolved Hide resolved

rafaelramalho19 requested a review from jessicaschilling November 6, 2020 16:51

jessicaschilling suggested changes Nov 6, 2020

View reviewed changes

lidel changed the title ~~Feat/pinning services stage 1~~ feat: pinning services stage 1 Nov 6, 2020

lidel requested changes Nov 6, 2020

View reviewed changes

lidel mentioned this pull request Nov 9, 2020

epic: pinning services #1615

Closed

jessicaschilling suggested changes Nov 9, 2020

View reviewed changes

public/locales/en/files.json Outdated Show resolved Hide resolved

jessicaschilling approved these changes Nov 9, 2020

View reviewed changes

rafaelramalho19 requested a review from lidel November 9, 2020 17:14

lidel mentioned this pull request Nov 10, 2020

Files screen: add pinning option to bottom bar when multiple files selected #1682

Open

lidel requested changes Nov 10, 2020

View reviewed changes

rafaelramalho19 and others added 10 commits November 10, 2020 15:06

Feat/pinning settings (#1535)

aaf0a8f

Feat/pinning settings custom modal (#1546)

327df82

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

Feat/redesign files bar (#1513)

5fcb1ba

Feat/pinning settings (#1535) (#1557)

11b9c69

* feat: add pinning services service modal * chore: update translation for service modal

chore: fix tslint in directory selector

9cbf791

chore: fix tscheck issues in upper directory selector

1296778

chore: fix tslint in selectors

14d26f0

feat: prepare pinning services for stage 1

820e234

chore: remove remote pins from settings page for stage 1

aeb9d0b

rafaelramalho19 and others added 15 commits November 10, 2020 15:06

chore: update pinning manager padding

92966af

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

chore: update modal horizontal padding

685b257

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

chore: make settings page pinning table responsive

b2e3c94

chore: update settings page description in pinning table

dc0d642

chore: update header style

9fa7b96

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

chore: update settings page in smaller viewports

e188bfd

chore: remove outline on pinning tables focus

7591194

chore: update translation

747dd6c

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

chore: update translation

0836a6f

Co-authored-by: Jessica Schilling <jessica@protocol.ai>

chore: add titles to bar options

29fccf4

chore: add titles to bar options

12c9313

Update public/locales/en/files.json

90f5bf1

feat: add pins size to the settings page

02a0ead

feat: add number of pins to settings page

6860cf0

chore: feat tslint

3f5c51b

rafaelramalho19 force-pushed the feat/pinning-services-stage-1 branch from 0b7769d to 3f5c51b Compare November 10, 2020 15:06

rafaelramalho19 and others added 2 commits November 16, 2020 18:37

chore: fix error in button

e59f689

lidel approved these changes Dec 14, 2020

View reviewed changes

rafaelramalho19 merged commit 2a312a3 into master Dec 14, 2020

rafaelramalho19 deleted the feat/pinning-services-stage-1 branch December 14, 2020 14:57

lidel mentioned this pull request Jan 5, 2021

Files: import drawer opens without a reason #1704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pinning services stage 1 #1685

feat: pinning services stage 1 #1685

rafaelramalho19 commented Nov 4, 2020 •

edited by jessicaschilling

Loading

jessicaschilling left a comment

rafaelramalho19 commented Nov 5, 2020

jessicaschilling commented Nov 5, 2020 •

edited

Loading

jessicaschilling left a comment

jessicaschilling left a comment

lidel left a comment

rafaelramalho19 commented Nov 9, 2020

lidel left a comment

lidel commented Nov 11, 2020 •

edited

Loading

rafaelramalho19 commented Nov 12, 2020

lidel commented Nov 13, 2020

lidel left a comment •

edited

Loading

feat: pinning services stage 1 #1685

feat: pinning services stage 1 #1685

Conversation

rafaelramalho19 commented Nov 4, 2020 • edited by jessicaschilling Loading

jessicaschilling left a comment

Choose a reason for hiding this comment

rafaelramalho19 commented Nov 5, 2020

jessicaschilling commented Nov 5, 2020 • edited Loading

jessicaschilling left a comment

Choose a reason for hiding this comment

jessicaschilling left a comment

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment

rafaelramalho19 commented Nov 9, 2020

lidel left a comment

Choose a reason for hiding this comment

lidel commented Nov 11, 2020 • edited Loading

rafaelramalho19 commented Nov 12, 2020

lidel commented Nov 13, 2020

lidel left a comment • edited Loading

Choose a reason for hiding this comment

rafaelramalho19 commented Nov 4, 2020 •

edited by jessicaschilling

Loading

jessicaschilling commented Nov 5, 2020 •

edited

Loading

lidel commented Nov 11, 2020 •

edited

Loading

lidel left a comment •

edited

Loading