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

feat: add inspect near browse #1114

Merged
merged 5 commits into from
Aug 20, 2019
Merged

feat: add inspect near browse #1114

merged 5 commits into from
Aug 20, 2019

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Aug 16, 2019

Close #1109. Closes #1112. #1027 introduced a regression: we replaced the IPLD explore bar by the files Explore bar. Although, we didn't put the IPLD explore bar anywhere else.

This PR makes the top bar work for both Files and IPLD sections. How? Like this:

Untitled

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 requested review from olizilla and lidel August 16, 2019 10:20
@olizilla
Copy link
Member

That is snazzy, but perhaps it could be much simpler.

  • if the CID is point to a node that is not a dag-pb unixfs thing, the explore button should open it in the IPLD explore page, otherwise open it in the files page.
  • There should be a clear option in the files page to view the current connent in the ipld explorer. I think we already have that in the context menu and the action bar, so perhaps that is already covered.

@olizilla
Copy link
Member

if you are on the ipld explore page, that bar should open what you search for in the ipld page.

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.

Potential UX problem:

  • menu on the left of the screen has "Explore" button pointing at "Explore screen"
  • search box at the top has "Explore" on a button that opens CID in "Files screen" instead of "Explore screen", and "Inspect" button that opens in "Explore screen" 🙃
    • I would expect label opening in "Files" to be something like "Browse"
    • OR rename "Explore" screen in the left menu to "Inspector" (reusing mental model of HTML inspector in web browsers)

@hacdias
Copy link
Member Author

hacdias commented Aug 16, 2019

@olizilla

if the CID is point to a node that is not a dag-pb unixfs thing, the explore button should open it in the IPLD explore page, otherwise open it in the files page.

I thought about that first, although that would require to resolve possible /ipns domains and resolve whole paths such as /HashToCbor/but/this/is/unixfs.

There should be a clear option in the files page to view the current connent in the ipld explorer. I think we already have that in the context menu and the action bar, so perhaps that is already covered.

Yes, already possible.

@lidel yes, using 'Browse'/'Explore' is the best IMO. Will change.

@hacdias hacdias requested a review from lidel August 16, 2019 11:08
@hacdias hacdias mentioned this pull request Aug 16, 2019
16 tasks
@hacdias
Copy link
Member Author

hacdias commented Aug 19, 2019

Ping @olizilla @lidel

@lidel
Copy link
Member

lidel commented Aug 19, 2019

Handle "Browse" and non-unixfs

Clicking on "Browse" for non-unixfs node (eg. bafyreiddymapg5zcpma3iu4wingqvois6jirucn5776wdsyg5f3f65v75a) produces error.

@hacdias Are you able to rebase this PR on top of master (to include fixes from #1115)?

I want to confirm user will see below message instead of an error :)

Proof-read English text

SGTM, but let's get 👍 from a native speaker (@olizilla / @autonome)

@hacdias
Copy link
Member Author

hacdias commented Aug 19, 2019

@lidel just merged with master! Remember that now it will only show up if you go to the specific link! If you're just navigating, you'll be automatically taken to the Inspector. Seems to work as expected!

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.

Inspect feature LGTM.

Sidenote (can be done in a separate PR): just like @ericronne suggested last week, we should add color to the input and buttons on mouse hover, otherwise it looks like disabled UI element.

@ericronne
Copy link

Yep. Actually any interactive element should look clickable by default, otherwise users may not ever know to hover over it. Here are two ways we could telegraph clickability in regards to this feature set …

image

ps wearing my "user hat," i'm still not sure what to make of the house icon. My brain expects that to be a number, in keeping with the pattern set by the blocks and pins counts. The word "files" may be part of the problem, as it suggests "X files" 👽 (number of files) to me.

Would changing the label to "home" make sense?

@lidel
Copy link
Member

lidel commented Aug 20, 2019

No, "home" feels out of place. Now that I think about it, do we need the "files" as a navigation item in the section on the right at all?

Clickable "files" is already present as the root of breadcrumbs on the left:
2019-08-20--20-00-41

My intuition is to remove it or replace clickable icon with non-clickable count.
It could be total size of files in MFS (CumulativeSize from ipfs files stat /

Thoughts?

@hacdias
Copy link
Member Author

hacdias commented Aug 20, 2019

@ericronne @lidel moving that conversation to #1122.

@hacdias hacdias merged commit ecad455 into master Aug 20, 2019
@hacdias hacdias deleted the feat/add-inspect-browse branch August 20, 2019 19:51
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.

Can't ipld-explore anymore
4 participants