Feature - CLI Tutor Mode #1572

jay-dee7 · 2020-08-01T18:33:49Z

Adds the CLI Tutor Mode, which can help with learning the IPFS Command Line Interface from the Webui/Desktop. This feature was requested in #1421

Done

Option to enable/disable CLI Tutor Mode from the settings page
If enabled, a copy-icon 📋 is shown next to the buttons/menus where those actions can be performed via IPFS' CLI
This icon is visible at:
- Files Page -> inside + Import menu (Add File, Folder, Copy from IFPS)
- Files Page -> File Menu (three dots next to a file) -> (Delete, Rename, Pin/Unpin, Download)
- Peers Page -> next to + Add Connection
- Settings Page -> next to Reset button in config edit box
- ~~Setting Page -> next to Submit button in API edit box~~

Pending Work:

~~I recently discovered a state management issue with the modals, I couldn't find the root cause so far, it started happening after the refactor today (1st Aug 2020)~~
~~After rebasing from master today, there's a small UI glitch (text in the modal actions is left aligned, I'll work it out before making this PR reviewable)~~
~~I have added a bunch of TODO's in the code, which can potentially help with the code cleanup~~
~~Files component kind of looks like a mess right now, a lot of state is being shared across mulitple components (maybe we can refactor it to make it look neat)~~

Here's a visual for how it looks in action: [Jessica note: these are a bit outdated now since commits for visual tweaks on 12 Aug]

Settings Page | Option to enable CLI Tutor Mode

Files Page | CLI Tutor Mode Enabled

Settings Page | CLI Tutor Mode - Open Modal

Signed-off-by: Jasdeep Singh jasdeepsingh.uppal@gmail.com

jay-dee7 · 2020-08-01T18:37:59Z

@jessicaschilling here's the draft for cli tutor mode, the place which would need more attention is files component, since it has a lot of shared state, please check this out and suggest any refactors/changes for places where i might have broken any acceptable coding patterns. I'll update this PR tomorrow with little of code cleanup and modal actions alignment fix

jessicaschilling · 2020-08-01T18:58:21Z

Thanks, @jay-dee7! I'd like @rafaelramalho19 to look through this in more detail ... but just a heads up that he's on holiday for the next week. Don't want you to think that we are ignoring you. 😊

jay-dee7 · 2020-08-01T19:17:08Z

That's okay @jessicaschilling. In-fact apologies for the delay. This should've been up way before than it did. In the meantime I'll keep on iterating over this, just to see if anything can be improved upon 😄

jessicaschilling

A few quick thoughts before @rafaelramalho19 reviews; two review comments inline for text changes, too:

Can you please center text in the "Close" and "Copy" buttons in the modal?
For the example shown (IPFS config file) - if the user just pastes ipfs config replace settings.json into their terminal, this won't actually work, because there's no context of settings.json. Or is this what you mean by the download icon - is that supposed to download the changes made in the window to a file settings.json? If it's the latter, we'll need to make that more clear (and there will still be issues, because just "settings.json" won't necessarily be the right directory level).
Overall, can you please make the size of the clipboard icon a little smaller, based on a vertical height to visually match the vertical height of the trash can icon in the three-dots context menu? (Understood that this might be numerically smaller than the trash can vertically due to inconsistencies in icon aspect ratios.)

I'll have a deeper look when you're done with your cleanup, too. 😊 Thanks!

src/components/cli-tutor-mode/CliTutorMode.js

jay-dee7 · 2020-08-04T04:12:11Z

sure @jessicaschilling i'll make these changes ASAP. There are more things that I didn't notice initially. ContextMenu in files pages needs some work, after rebasing, the copy icon and the text aren't aligned symmetrically and some of the commands aren't correct (as in rename command in files page). I'll need some sort of verification if the commands that i did add are the correct ones.

jay-dee7 · 2020-08-09T14:39:15Z

@jessicaschilling I rebased from master just a while ago, one of the incoming changes were in ApiAddressForm component and since it was written using hooks (I believe), I was not able to overwrite props easily (I'm not sure what exactly broke the prop sharing, i believe its hooks but i have no idea.). So just for the ApiAddressForm component, I've managed the state for cli-tutor-modal locally to the component, instead of using the regular redux actions. Rest of the stuff stays almost same. I think this PR is ready for review 🤞

jessicaschilling · 2020-08-09T15:32:12Z

Thanks, @jay-dee7!
@rafaelramalho19 - are you able to take a look at this PR once you are back in and settled? Thank you!

src/bundles/cli-tutor-mode.js

rafaelramalho19 · 2020-08-10T17:48:27Z

src/components/api-address-form/ApiAddressForm.js

+        <CopyIcon
+          onClick={setCliTutorModal}
+          className='dib fill-current-color ph2 glow o-80 pointer'
+          style={{ height: '28px', transform: 'scale(1.5)', verticalAlign: 'bottom', color: 'dodgerblue' }}


This scale can lead to some issues in Safari. Can you try and make it without the scale?

for this, i have now added two options, one using just the bigger pixel size (height is increased) and another one using msTransform and webkitTransform. I can quickly stick to one solution but as far as i can tell, they both work as expected. webkitTransform worked okay for me on safari too

It works, the problem is that it renders a pixelated icon in safari (old issue unfortunately), so I would just stick with increasing the height

src/components/cli-tutor-mode/CliTutorMode.js

src/files/FilesPage.js

rafaelramalho19 · 2020-08-10T18:16:22Z

src/files/dropdown/Dropdown.js

+      </button>
+      <button {...props} className={`bg-animate hover-bg-near-white pa2 pointer flex items-center  ${className}`}>
+        <CopyIcon {...props} onClick={onCliTutorMode} className='dib fill-current-color ph2 glow o-80 pointer'
+          style={{ height: '28px', transform: 'scale(1.3)', color: 'dodgerblue' }}


Try to avoid the transform (same as commented before)

added a css class for this with webkitTransform but it would also work if we simple increased the height by 1.3X. Lemme know which way we'd like to move forward with :)

src/settings/SettingsPage.js

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

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

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

rafaelramalho19 · 2020-08-12T11:46:37Z

After you remove the transforms, LGTM 😄

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

jay-dee7 · 2020-08-12T16:38:55Z

thanks for the amazing review @rafaelramalho19, i've change the css class to use just height and no transforms :)

jessicaschilling · 2020-08-14T17:24:07Z

@jay-dee7 @lidel I'd vote for closing the dialog onclick of the "Copy" button. @jay-dee7, can you please change that?

jay-dee7 · 2020-08-14T17:27:39Z

Sure @jessicaschilling I'll make the change ASAP :)

jessicaschilling · 2020-08-14T17:28:28Z

@jay-dee7 And thanks for spotting the misaligned checkbox! Fixed it. 😊

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

…bui into jay-dee7/cli-tutor-mode

jay-dee7 · 2020-08-15T08:33:50Z

@jessicaschilling @lidel i've added both the requested points.
Now by clicking the copy button in the modal, we copy the contents (command) and then close the modal.

Also, I've added a fix for rename command. I realized that we need to preserve the file path while renaming and we should wrap the full path path with quotes (""), because it'll throw errors if the full path contains spaces (for example a file named cat image.png would throw error in cli but if we wrap it with quotes as "cat image.png") it won't. Please let me know if we're good to go with this :)

jessicaschilling · 2020-08-17T17:28:38Z

@lidel - I'm happy but will wait for your OK, please feel free to merge 😊

lidel

@jay-dee7 nearly there! small asks inline 🙏

src/bundles/files/consts.js

tsconfig.json

src/files/modals/Modals.js

…dList Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

Without this below will fail: ipfs files rm -r /test/white space/flowers.jpeg

This removed CLI tutor from "API Adddress" selector because it does not make sense: "API ADDRESS" section does not change the config of IPFS node, but defines the API which WebUI will use when connecting to the node.

text between < and > won't be translated because Transifex blackboxes HTML tags, it is better to remove it

lidel

I made small tweaks in 9821bc8, 7d71641 and 8cc6d85 and run all tests locally (because CI missed them: #1595).

Everything looks ok, but i'm afk till Tuesday so leaving this open in case someone has time to tackle i18n issue described below.
If not, ok to merge and tackle the problem in separate PR.

Note on i18n: Duplicated of translation keys (cliModal and actions) are not the best, but fallbackNS does not work correctly in i18next for some reason, and I understand why it has to be like this for now. I spent some time today trying to find a fix but it proved to be huge time sink and a rabbit hole of i18next internals: we may need to leave duplicated keys as they are and revisit in separate PR.

Transifex will automatically fill all places when at least one of them is translated, so in practice it should not be a problem, and it will be easy to deduplicate keys in the future, if the issue is resolved upstream.

See https://react.i18next.com/guides/multiple-translation-files

jessicaschilling · 2020-08-21T18:55:22Z

@lidel — Unless I'm missing something, I believe 881bcbf and 84b6cf4 take care of this ... per i18next docs we can explicitly reference a different namespace than the default one we declare for a given file.

I added the i18n keys for cliModal and the actions buttons to app.json since it looks like at one point that was actually intended as a highest-level or common translation file.

If this approach makes sense to you, there's an opportunity to remove other duplicate keys throughout the project — especially in the tour and actions categories — and point them at app.json, but that's off-topic enough to merit being done as a separate effort.

The way import via Webui works is: 1. `ipfs add` produces CID 2. we create a reference to that CID in MFS We do this to produce the same CID as regular ipfs add from CLI would do, mainly to avoid issue described in: ipfs#676

Right now CLI tutor defaults to MFS root at /, this makes it easier to include current path in the future.

lidel

Thank you @jessicaschilling, I think the old i18next issue is gone, all looks good.

I've made a small fix in 5641184 to ensure CLI tutor lists import actions as the button in GUI
(ipfs add && ipfs files cp, context: #676)

nit: CLI tutor for Import commands ignores the fact we may be in a subdirectory.

Ideally /<dest-name> in import commands would be replaced with "${path}/<dest-name>" where path is the current dir in MFS. I started doing that in 3a337de but had no bandwidth to wire it up.

I think it's ok to merge with static destination, but if anyone has time to ensure path is set for these actions that would be even better for user experience.

jessicaschilling · 2020-08-26T18:29:06Z

@lidel -- Thanks for calling the subdirectory detail out. I'm inclined to merge this as-is and address that in a separate issue (#1601), since I suspect we may want to make some tweaks to tutor mode overall after this gets into the hands of users for a bit.

I've also opened #1600 for consolidating repeat-use i18n keys, which I can probably take care of in the next week or two.

Thanks all (especially @jay-dee7!) for all of the work!

jessicaschilling requested review from lidel and rafaelramalho19 August 3, 2020 20:24

jessicaschilling suggested changes Aug 3, 2020

View reviewed changes

src/components/cli-tutor-mode/CliTutorMode.js Outdated Show resolved Hide resolved

src/components/cli-tutor-mode/CliTutorMode.js Outdated Show resolved Hide resolved

jay-dee7 requested a review from jessicaschilling August 5, 2020 01:49

jay-dee7 force-pushed the jay-dee7/cli-tutor-mode branch from cb8ded0 to cd222ef Compare August 9, 2020 14:31

jay-dee7 marked this pull request as ready for review August 9, 2020 15:35

jay-dee7 changed the title ~~[WIP] Feature - CLI Tutor Mode~~ Feature - CLI Tutor Mode Aug 10, 2020

rafaelramalho19 suggested changes Aug 10, 2020

View reviewed changes

jay-dee7 and others added 5 commits August 11, 2020 00:12

[WIP] Feature - CLI Tutor Mode

0531b14

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

Update src/components/cli-tutor-mode/CliTutorMode.js

a6c350a

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

Update src/components/cli-tutor-mode/CliTutorMode.js

3553611

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

Align fix: cli tutor modal buttons

c8b4034

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

Fix: added compatibility with ApiAddressForm component

da31e1c

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

jay-dee7 force-pushed the jay-dee7/cli-tutor-mode branch 2 times, most recently from 8890925 to 6ff45e4 Compare August 11, 2020 21:19

rafaelramalho19 approved these changes Aug 12, 2020

View reviewed changes

Fix: added bundle-reactx pattern and en translation keys

97a06b0

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

jay-dee7 force-pushed the jay-dee7/cli-tutor-mode branch from 6ff45e4 to 97a06b0 Compare August 12, 2020 16:37

jessicaschilling added 4 commits August 12, 2020 13:42

i18n text change

7d018c3

Standardize i18n and visual style on Settings page

942bfaf

Text tweak; replace CliTutorMode.css with tachyons equivalents

8d59332

Left-justify shell content in modals for consistency

7b7f361

Move lh-copy so doesn't fubar checkbox styling

c291643

jessicaschilling added this to the v2.11 milestone Aug 14, 2020

jay-dee7 added 3 commits August 15, 2020 13:53

Fix: Close modal on copy to clipboard

3ee8432

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

Fix: rename file command

15d6578

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

Merge branch 'jay-dee7/cli-tutor-mode' of github.com:jay-dee7/ipfs-we…

ce41d30

…bui into jay-dee7/cli-tutor-mode

jay-dee7 requested a review from lidel August 20, 2020 09:37

lidel requested changes Aug 20, 2020

View reviewed changes

src/bundles/files/consts.js Outdated Show resolved Hide resolved

tsconfig.json Outdated Show resolved Hide resolved

src/files/modals/Modals.js Show resolved Hide resolved

jay-dee7 and others added 4 commits August 21, 2020 00:44

Fix: delete file by filepath and added type information for cliComman…

b761d7d

…dList Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>

fix: whitespace in ipfs files rm

9821bc8

Without this below will fail: ipfs files rm -r /test/white space/flowers.jpeg

fix: remove UPDATE_API_SERVER_ADDRESS

7d71641

This removed CLI tutor from "API Adddress" selector because it does not make sense: "API ADDRESS" section does not change the config of IPFS node, but defines the API which WebUI will use when connecting to the node.

fix: eslint

72a7ab7

lidel mentioned this pull request Aug 21, 2020

Move to Github Actions #1595

Closed

4 tasks

fix: remove angle brackets from i18n strings

8cc6d85

text between < and > won't be translated because Transifex blackboxes HTML tags, it is better to remove it

lidel approved these changes Aug 21, 2020

View reviewed changes

jessicaschilling added 2 commits August 21, 2020 12:32

Move cliModal i18n keys into app.json

881bcbf

See https://react.i18next.com/guides/multiple-translation-files

Move copyCommand & relevant close i18n keys into app.json

84b6cf4

jessicaschilling requested a review from lidel August 21, 2020 18:57

lidel added 2 commits August 26, 2020 15:43

fix: include files cp step

5641184

The way import via Webui works is: 1. `ipfs add` produces CID 2. we create a reference to that CID in MFS We do this to produce the same CID as regular ipfs add from CLI would do, mainly to avoid issue described in: ipfs#676

refactor: make it easier to support path

3a337de

Right now CLI tutor defaults to MFS root at /, this makes it easier to include current path in the future.

lidel approved these changes Aug 26, 2020

View reviewed changes

This was referenced Aug 26, 2020

Consolidate repeat-use i18n keys #1600

Closed

CLI tutor mode: Support subdirectories for "import" commands #1601

Closed

jessicaschilling merged commit b08b14e into ipfs:master Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature - CLI Tutor Mode #1572

Feature - CLI Tutor Mode #1572

jay-dee7 commented Aug 1, 2020 •

edited by lidel

jay-dee7 commented Aug 1, 2020

jessicaschilling commented Aug 1, 2020

jay-dee7 commented Aug 1, 2020

jessicaschilling left a comment

jay-dee7 commented Aug 4, 2020 •

edited

jay-dee7 commented Aug 9, 2020

jessicaschilling commented Aug 9, 2020

rafaelramalho19 Aug 10, 2020

jay-dee7 Aug 11, 2020

rafaelramalho19 Aug 12, 2020

rafaelramalho19 Aug 10, 2020

jay-dee7 Aug 11, 2020

rafaelramalho19 commented Aug 12, 2020

jay-dee7 commented Aug 12, 2020

jessicaschilling commented Aug 14, 2020

jay-dee7 commented Aug 14, 2020

jessicaschilling commented Aug 14, 2020

jay-dee7 commented Aug 15, 2020

jessicaschilling commented Aug 17, 2020

lidel left a comment •

edited

lidel left a comment •

edited

jessicaschilling commented Aug 21, 2020 •

edited

lidel left a comment •

edited

jessicaschilling commented Aug 26, 2020

Feature - CLI Tutor Mode #1572

Feature - CLI Tutor Mode #1572

Conversation

jay-dee7 commented Aug 1, 2020 • edited by lidel

Done

Pending Work:

Settings Page | Option to enable CLI Tutor Mode

Files Page | CLI Tutor Mode Enabled

Settings Page | CLI Tutor Mode - Open Modal

jay-dee7 commented Aug 1, 2020

jessicaschilling commented Aug 1, 2020

jay-dee7 commented Aug 1, 2020

jessicaschilling left a comment

Choose a reason for hiding this comment

jay-dee7 commented Aug 4, 2020 • edited

jay-dee7 commented Aug 9, 2020

jessicaschilling commented Aug 9, 2020

rafaelramalho19 Aug 10, 2020

Choose a reason for hiding this comment

jay-dee7 Aug 11, 2020

Choose a reason for hiding this comment

rafaelramalho19 Aug 12, 2020

Choose a reason for hiding this comment

rafaelramalho19 Aug 10, 2020

Choose a reason for hiding this comment

jay-dee7 Aug 11, 2020

Choose a reason for hiding this comment

rafaelramalho19 commented Aug 12, 2020

jay-dee7 commented Aug 12, 2020

jessicaschilling commented Aug 14, 2020

jay-dee7 commented Aug 14, 2020

jessicaschilling commented Aug 14, 2020

jay-dee7 commented Aug 15, 2020

jessicaschilling commented Aug 17, 2020

lidel left a comment • edited

Choose a reason for hiding this comment

lidel left a comment • edited

Choose a reason for hiding this comment

jessicaschilling commented Aug 21, 2020 • edited

lidel left a comment • edited

Choose a reason for hiding this comment

jessicaschilling commented Aug 26, 2020

jay-dee7 commented Aug 1, 2020 •

edited by lidel

jay-dee7 commented Aug 4, 2020 •

edited

lidel left a comment •

edited

lidel left a comment •

edited

jessicaschilling commented Aug 21, 2020 •

edited

lidel left a comment •

edited