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

chore: update add dropdown #951

Merged
merged 3 commits into from
Feb 6, 2019
Merged

chore: update add dropdown #951

merged 3 commits into from
Feb 6, 2019

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented Feb 4, 2019

As per #915 (comment), I've refactored the add dropdown. Heavily inspired by Drive and Dropbox.

  • Separate the create new folder from the add to ipfs dropdown. This way, everything inside that dropdow is to add file/folder/cid to our repo.
  • Renamed the dropdown to just + Add. Previously we had + Add to IPFS, which is not 100% correct. This let me update the Add by path which was super confusing, to Add from IPFS, which I think is more clear because it tells you you're adding something that is already on IPFS.

1

2

Closes #915.

@fsdiogo fsdiogo self-assigned this Feb 4, 2019
@ghost ghost added the status/in-progress In progress label Feb 4, 2019
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.

Good idea. Works well. Needs moar <Button/>

src/files/file-input/FileInput.js Outdated Show resolved Hide resolved
src/files/FilesPage.js Outdated Show resolved Hide resolved
src/files/FilesPage.js Outdated Show resolved Hide resolved
License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost assigned olizilla Feb 6, 2019
@olizilla
Copy link
Member

olizilla commented Feb 6, 2019

I tweaked the AddButton to remove the i18n and lng props as they were being passed through to the dom element. I tweaked the new file button to use v-mid on the svg rather than flex, and wrapped the text in its own span with a fw3

@fsdiogo fsdiogo merged commit 704d896 into master Feb 6, 2019
@ghost ghost removed the status/in-progress In progress label Feb 6, 2019
@fsdiogo fsdiogo deleted the chore/update-add-dropdown branch February 6, 2019 10:23
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.

All proposed changes make things better 👌

Added some random thoughts below, but can be merged as is.

"addByPath": "Add by path",
"addToIPFS": "Add to IPFS",
"addFile": "File",
"addFolder": "Folder",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps

Folder with files

?

"addToIPFS": "Add to IPFS",
"addFile": "File",
"addFolder": "Folder",
"addByPath": "From IPFS",
Copy link
Member

@lidel lidel Feb 6, 2019

Choose a reason for hiding this comment

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

May be just me, but perhaps more explicit styling of "IPFS" as a path would make it more clear:

From /ipfs/…

@terichadbourne
Copy link

A couple of suggestions/observations made on our call today:

  • Under the option to add from IPFS, perhaps we could reference options more specific than "path," like "CID" or "IPNS path," and provide examples of acceptable formats.
  • For me personally, "Add" doesn't imply that the thing you're adding is pre-existing, whereas "Upload" or "Import" do (I know you have technical objections to those terms), so leaving it verbless doesn't help me know that you're referring to existing files/folders instead of blank ones

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Feb 8, 2019

@lidel @terichadbourne what do you think:

from-ipfs

@terichadbourne
Copy link

@fsdiogo Yes, that’s much easier for me to understand!

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.

4 participants