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

💄: files tweaks #723

Merged
merged 18 commits into from
Jul 17, 2018
Merged

💄: files tweaks #723

merged 18 commits into from
Jul 17, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jul 10, 2018

Blocked by:

Changes:

  • Removes 'status' column for now since it's not yet used
  • Status and Size disappear on small screens
  • New file icon
  • Adds the hash behind the name
  • Improve tooltip component
  • when clicking the hash you go to inspect
  • Adding button shows progress

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2018

Progress so far:

localhost_3000_ 2

@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2018

Questions for you (@lidel @olizilla @alanshaw @fsdiogo).

Should we just use the native tooltip through title attr? Why asking this? Because the hash currently doesn't have a tooltip and it might never have, but we could also apply this tooltip to the hash. What's your thoughts on this?

image

Go bold or don't go bold?

image

Go Smaller or Don't Go Smaller?

Smaller with Bold:

image

Smaller without Bold:

image

Clicking on the hash should copy the hash, go to Inspect or just open the file as clicking on the name would do?

@hacdias hacdias self-assigned this Jul 10, 2018
@hacdias hacdias mentioned this pull request Jul 10, 2018
18 tasks
@lidel
Copy link
Member

lidel commented Jul 10, 2018

I'd give tooltip to both hash and filename, as we may have very long ones that get truncated, and tooltip should have the full version.

As for file listing, I like first and last version, but last one may be easier on the eyes.
Perhaps we could bold filename only when it is selected via checkbox?

@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2018

@lidel,

I'd give tooltip to both hash and filename, as we may have very long ones that get truncated, and tooltip should have the full version.

Yes, makes sense.

As for file listing, I like first and last version, but last one may be easier on the eyes. Perhaps we could bold filename only when it is selected via checkbox?

So you prefer the smaller font-size with bold when checked, right?

A question:

Clicking on the hash should copy the hash, go to Inspect or just open the file as clicking on the name would do?

@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2018

Hmm, Lighthouse says any font below 16px is small. Using f6 for the name and size, they end up with 14px.

@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2018

image

A suggestion for upload progress instead of adding the files directly to the list. What do you think? Which one seems better? This or adding the pending files on the file list?

@lidel
Copy link
Member

lidel commented Jul 10, 2018

As for file listing, I like first and last version, but last one may be easier on the eyes. Perhaps we could bold filename only when it is selected via checkbox?
So you prefer the smaller font-size with bold when checked, right?

Actually I have no preference on the font size, just don't think we should bold all the time, but only when selected.

Clicking on the hash should copy the hash, go to Inspect or just open the file as clicking on the name would do?

Very good question. Personally I would expect it to copy, as it is the most common action I do with CIDs, but it may be a developer bias. I feel this needs more interviews / data points :)

image
A suggestion for upload progress instead of adding the files directly to the list. What do you think?

I like it because it "is good enough for now" and solves problem of reporting progress on multiple files and cancelling entire operation.
Perhaps label could be replaced with % progress, to be in line with the way downloads work right now?
Following that convention, clicking on it during upload would cancel the upload and restore original label.

@hacdias
Copy link
Member Author

hacdias commented Jul 10, 2018

Very good question. Personally I would expect it to copy, as it is the most common action I do with CIDs, but it may be a developer bias. I feel this needs more interviews / data points :)

Sure, makes sense. let's wait for more opinions.

I like it because it solves problem of reporting progress on multiple files and cancelling entire operation.
Perhaps label could be replaced with % progress, to be in line with the way downloads work right now?
Following that convention, clicking on it during upload would cancel the upload and restore original label.

Didn't know we could cancel the add process and I can't find info about it. Either way, cancel it should just stop the process and leave everything as-is or shall we also remove the already added files?

@lidel
Copy link
Member

lidel commented Jul 10, 2018

Good catch :) I forgot we don't have cancellable API requests due to Fetch API limitations: https://github.com/ipfs/js-ipfs-api/issues/694
Ignore cancellation then (needs to be non-cancellable for now)

@hacdias
Copy link
Member Author

hacdias commented Jul 11, 2018

image

@hacdias hacdias changed the title [wip] 💄: files tweaks 💄: files tweaks Jul 11, 2018
@hacdias
Copy link
Member Author

hacdias commented Jul 11, 2018

I think this is ready to be reviewed. The CI is failing but it seems to be failing randomly at the navigation test. Should we change the timeout?

@hacdias hacdias requested review from alanshaw and lidel and removed request for alanshaw July 11, 2018 08:38
@hacdias hacdias requested review from olizilla, lidel, alanshaw and fsdiogo and removed request for lidel and olizilla July 11, 2018 08:38
@hacdias
Copy link
Member Author

hacdias commented Jul 11, 2018

A bit better on smaller screens:

localhost_3000_ 3

localhost_3000_ 4

Altough I find that the sidebar should be collapsed by default when screen size <= 60em.

@hacdias
Copy link
Member Author

hacdias commented Jul 17, 2018

Can we merge this @lidel @olizilla @alanshaw @fsdiogo ?

@@ -11,21 +11,21 @@ import DocText from '../../icons/GlyphDocText'

export default function FileIcon ({name, type}) {
if (type === 'directory') {
return <Folder className=' fill-aqua' width='2.5rem' />
return <Folder className=' fill-aqua' style={{width: '2.5rem'}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

The style={{width: '2.5rem'}} should be in a variable as it's being used in all the cases.

Copy link
Contributor

@fsdiogo fsdiogo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hacdias
Copy link
Member Author

hacdias commented Jul 17, 2018

I'm merging this now.

@olizilla you said you had an issue with some loop that was fetching ls many times. If you find a way to reproduce it, could you post it on #704, please? 😄

@hacdias hacdias merged commit ba74dab into revamp Jul 17, 2018
@hacdias hacdias deleted the 💄/revamp/files-tweaks branch July 17, 2018 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants