[BOUNTY] Directory page UI improvements #7536

neatonk · 2020-07-12T04:27:06Z

BOUNTY details: ipfs/dir-index-html#37

These changes are needed to prepare for the Directory page UI improvements
implemented in ipfs/dir-index-html#38.

update dir-index-html type structs
emit CID of each directoryItem
emit size of directory
emit breadcrumbs

@jessicaschilling
@aschmahmann

jessicaschilling · 2020-07-13T14:25:42Z

Thanks, @neatonk! @aschmahmann -- can you please have a look as you're able?

lidel

@neatonk are you able to provide steps for testing your changes with real go-ipfs?

I tried:

updated .gitmodule to point at your fork

checked out version from [BOUNTY] Directory page UI improvements dir-index-html#38:

$ cd assets
$ git -C dir-index-html checkout c5d042ac5fd9c15631f95c6656e686a65f9f65ba

regenerated assets via go generate .
rebuilt go-ipfs
Opened /ipfs/QmdmQXB2mzChmMeKY47C43LxUdg1NDJ5MWcKMKxDu7RgQm/ from local gateway

And... I don't see the CID column described in ipfs/dir-index-html#38.

Am I missing anything?

neatonk · 2020-07-14T12:32:30Z

@lidel I think you need to actually commit the submodule change or the generate step will not use it.

lidel

Ack, after commiting I had to run go generate . again and then was able to build it – thanks @neatonk 👍

Found some bugs when loading listing via subdomain gateway:

For test URL http://bafybeihfg3d7rdltd43u3tfvncx7n5loqofbsobojcadtmokrljfthuc7y.ipfs.localhost:8080/1%20-%20Barrel%20-%20Part%201 none of newly added links work correctly because they use content paths under subdomain:

To illustrate:

http://bafybeihfg3d7rdltd43u3tfvncx7n5loqofbsobojcadtmokrljfthuc7y.ipfs.localhost:8080/ipfs/bafybeihfg3d7rdltd43u3tfvncx7n5loqofbsobojcadtmokrljfthuc7y/1%20-%20Barrel%20-%20Part%201 is broken because it should point at either:
- http://localhost:8080/ipfs/bafybeihfg3d7rdltd43u3tfvncx7n5loqofbsobojcadtmokrljfthuc7y/1%20-%20Barrel%20-%20Part%201 (preferred – will redirect user to correct subdomain)
- or http://bafybeihfg3d7rdltd43u3tfvncx7n5loqofbsobojcadtmokrljfthuc7y.ipfs.localhost:8080/1%20-%20Barrel%20-%20Part%201 (fine, but previous one is better)

neatonk · 2020-07-15T04:32:53Z

Thanks @lidel! I'll update the links.

jessicaschilling · 2020-07-21T22:20:01Z

@lidel, looks like this is ready for your review when you're able. Thanks!

lidel

Thank you @neatonk, listings on subdomains now work as expected, I also tested DNSLink gateway mode and works fine too 👍

Overall lgtm from functional perspective, but found small nits that need to be addressed (details inline)

Summary:

💚 http://bafybeihfg3d7rdltd43u3tfvncx7n5loqofbsobojcadtmokrljfthuc7y.ipfs.localhost:8080/
💚 http://ipfs.io.ipns.localhost:8080/images (but 💔 for /ipns/ being a broken link)
💚 curl -H "Host: ipfs.io" -sD - "http://127.0.0.1:8080/images" (DNSLink gateway)
💔 http://bafybeig2ed6fx4mzkgopqafswqgjclffnscfphwsjmajt2otat5wc2zaim.ipfs.localhost:8080/foo/ipfs/bar (false-negative for subdomain breadcrumb)

core/corehttp/gateway_indexPage.go

lidel

Looks good so far! 💔 from my previous comment turned 💚 👍

Next steps for the bounty:

✅ review and merge [BOUNTY] Directory page UI improvements dir-index-html#38
✅ reconcile dir-index.html and dir-index-uncat.html: Reconcile dir-index.html and dir-index-uncat.html dir-index-html#39
✅ release dir-index-html
switch this PR to updated dir-index-html
- fix anything that breaks, eg. tests that match plain text without breadcrumb links (examples)

Not a part of the bounty, could be a separate PR:

add test/sharness/t0115-gateway-dir-index.sh (example)
- ensure dir-index includes proper size, hashes, and breadclumb links in path gateway and subdomains
- guard subdomain edge cases described in [BOUNTY] Directory page UI improvements #7536 (review)

jessicaschilling · 2020-07-23T23:30:00Z

ipfs/dir-index-html#39 takes care of reconciling dir-index.html and dir-index-uncat.html (plus typographic tweak suggested by @lidel in ipfs/dir-index-html#38 (comment)).

Regarding @lidel's comment above,

Not a part of the bounty, could be a separate PR:

@neatonk, not sure if adding that test is something you'd be interested in?

jessicaschilling · 2020-07-24T15:39:01Z

@neatonk @lidel Release 1.1.0 is available now, added some checkmarks to the list above: https://github.com/ipfs/dir-index-html/releases/tag/v1.1.0

jessicaschilling · 2020-07-24T23:39:58Z

Heads up: ipfs/dir-index-html#40 introduces a build script that, if it's the approach we want to take, may be worth cutting a new release for and using here (if only for efficiency).

neatonk · 2020-07-26T03:33:00Z

I'd argue for leaving ipfs/dir-index-html#40 out of this PR for simplicity.

neatonk · 2020-07-26T03:36:33Z

I will not be able to commit to adding test/sharness/t0115-gateway-dir-index.sh as I am running low on free time at the moment.

neatonk · 2020-07-27T03:28:32Z

@lidel I've updated this PR to use dir-index-html@v1.1.0 and fixed the gateway tests which match on the "Index of" lines. Apologies for not having time to add the sharness tests you mentioned.

jessicaschilling · 2020-07-27T21:30:30Z

New https://github.com/ipfs/dir-index-html/releases/tag/v1.2.0 should be applicable to straight-out sub in to this PR without any modifications, per ipfs/dir-index-html#40 (review) -- @lidel, up to you whether you want to do so. Thanks!

lidel · 2020-07-27T22:22:17Z

Switched to dir-index-html v1.2.0, will add quick sharness tests shortly.

This adds subdomain tests for breadcrumbs added in: ipfs#7536 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel

All green, thank you @neatonk for this contribution! ❤️

Listing CIDs next to files and clicable breadcrumbs will not only improve experience of everyday use for a lot of people, but more importantly, enable learning-by-exploration.

@aschmahmann I believe this is ready for your review. Most of the changes happened in assets/dir-index-html subdmodule, see ipfs/dir-index-html#38

Preview:

jessicaschilling · 2020-07-29T16:37:54Z

@neatonk - sent you a note re bounty!

aschmahmann

This looks great, thanks all for making this happen. @lidel I noticed some issues while trying to generate the bindata locally. It looks like we had an issue #7605 which @Stebalien fixed.

Do you mind rebasing on master and running go generate on the two commits that bump the submodule? I was going to do it myself and forcepush, but want to double check that we actually get the same results before merging anyway.

This regenerates bindata as noted in ipfs#7536 (review) License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

This adds subdomain tests for breadcrumbs added in: ipfs#7536 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel · 2020-08-19T12:26:16Z

@aschmahmann done! ready for your final take

FYSA rebasing this PR is indeed tricky: good call on double-checking.

go generate . relies on revision of assets/dir-index-html submodule and will discard any uncommited changes made to it. Unfortunately changing that revision was part of this PR, so you have uncommited diff while resolving rebase conflicts, which means a manual rebase will most likely fail/lose data.

To avoid that problem, I went with git rebase -Xours origin/master which automatically picked local version, without ever leaving the tree in "uncommited" state.

aschmahmann · 2020-08-19T23:31:29Z

assets/README.md

@@ -45,6 +45,7 @@ commit.
 ```bash
 > go generate .
 > git add bindata.go
+> git add bindata_version_hash.go


@lidel it looks like you forgot to run this while rebasing as I don't see any changes to the bindata_version_hash.go file.

@Stebalien merged #7609 and I rebased on top and added the relevant files. @lidel if you could double check in the morning that things look right that'd be great.

@aschmahmann retested locally and looks good, super happy about deterministic assets 👍

absolutely, it also helped with the fact that different OSes would give different modes.

These changes are needed to prepare for the Directory page UI improvements implemented in ipfs/dir-index-html#37. - update dir-index-html type structs - emit gateway URL for root links - emit CID of each directoryItem - emit size of directory - emit breadcrumbs

https://github.com/ipfs/dir-index-html/releases/tag/v1.2.0 also: https://github.com/ipfs/dir-index-html/releases/tag/v1.1.0 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

This adds subdomain tests for breadcrumbs added in: ipfs#7536 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

[BOUNTY] Directory page UI improvements This commit was moved from ipfs/kubo@e80601b

This comment has been minimized.

Sign in to view

jessicaschilling assigned aschmahmann Jul 13, 2020

jessicaschilling requested a review from aschmahmann July 13, 2020 14:25

lidel requested changes Jul 14, 2020

View reviewed changes

neatonk force-pushed the master branch from 2327777 to be143cf Compare July 15, 2020 04:14

neatonk force-pushed the master branch from be143cf to e717f72 Compare July 18, 2020 04:13

neatonk requested a review from lidel July 18, 2020 04:14

lidel requested changes Jul 21, 2020

View reviewed changes

core/corehttp/gateway_indexPage.go Outdated Show resolved Hide resolved

neatonk force-pushed the master branch from e717f72 to c2246e4 Compare July 23, 2020 02:12

lidel reviewed Jul 23, 2020

View reviewed changes

lidel added the topic/gateway Topic gateway label Jul 23, 2020

neatonk mentioned this pull request Jul 23, 2020

[BOUNTY] Directory page UI improvements ipfs/dir-index-html#38

Merged

jessicaschilling mentioned this pull request Jul 23, 2020

Reconcile dir-index.html and dir-index-uncat.html ipfs/dir-index-html#39

Merged

jessicaschilling mentioned this pull request Jul 23, 2020

chore: update dir-index-html to v1.0.6 #7511

Closed

jessicaschilling mentioned this pull request Jul 24, 2020

feat: Build script ipfs/dir-index-html#40

Merged

neatonk force-pushed the master branch from 5bbb7c8 to 8a37dea Compare July 27, 2020 03:14

neatonk requested a review from lidel July 27, 2020 03:23

lidel added a commit to neatonk/go-ipfs that referenced this pull request Jul 27, 2020

test(gateway): breadcrumb links in dir listing

1ec6403

This adds subdomain tests for breadcrumbs added in: ipfs#7536 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel approved these changes Jul 27, 2020

View reviewed changes

lidel mentioned this pull request Jul 27, 2020

Release 0.7.0 #7560

Closed

72 tasks

lidel added need/maintainers-input Needs input from the current maintainer(s) need/review Needs a review labels Aug 4, 2020

aschmahmann requested changes Aug 19, 2020

View reviewed changes

lidel added a commit to neatonk/go-ipfs that referenced this pull request Aug 19, 2020

chore: dir-index with go-bindata/v3

b2f57d6

This regenerates bindata as noted in ipfs#7536 (review) License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel force-pushed the master branch from b2f57d6 to d8412e1 Compare August 19, 2020 12:02

lidel added a commit to neatonk/go-ipfs that referenced this pull request Aug 19, 2020

test(gateway): breadcrumb links in dir listing

d8412e1

This adds subdomain tests for breadcrumbs added in: ipfs#7536 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel requested a review from aschmahmann August 19, 2020 12:26

aschmahmann reviewed Aug 19, 2020

View reviewed changes

Kevin Neaton and others added 5 commits August 19, 2020 20:09

chore: update dir-index-html to v1.1.0

9e95a82

test: update gateway tests for dir-index-html v1.1.0

2feff33

chore: dir-index-html v1.2.0

8ffb7ec

https://github.com/ipfs/dir-index-html/releases/tag/v1.2.0 also: https://github.com/ipfs/dir-index-html/releases/tag/v1.1.0 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

test(gateway): breadcrumb links in dir listing

887de89

This adds subdomain tests for breadcrumbs added in: ipfs#7536 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

aschmahmann force-pushed the master branch from d8412e1 to 887de89 Compare August 20, 2020 00:35

lidel requested a review from aschmahmann August 20, 2020 18:09

aschmahmann approved these changes Aug 20, 2020

View reviewed changes

aschmahmann merged commit e80601b into ipfs:master Aug 20, 2020

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#7536 from neatonk/master

3ed88d1

[BOUNTY] Directory page UI improvements This commit was moved from ipfs/kubo@e80601b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BOUNTY] Directory page UI improvements #7536

[BOUNTY] Directory page UI improvements #7536

neatonk commented Jul 12, 2020 •

edited by lidel

Loading

This comment has been minimized.

jessicaschilling commented Jul 13, 2020

lidel left a comment

neatonk commented Jul 14, 2020

lidel left a comment •

edited

Loading

neatonk commented Jul 15, 2020

jessicaschilling commented Jul 21, 2020

lidel left a comment •

edited

Loading

lidel left a comment •

edited by jessicaschilling

Loading

jessicaschilling commented Jul 23, 2020 •

edited

Loading

jessicaschilling commented Jul 24, 2020

jessicaschilling commented Jul 24, 2020

neatonk commented Jul 26, 2020

neatonk commented Jul 26, 2020

neatonk commented Jul 27, 2020

jessicaschilling commented Jul 27, 2020

lidel commented Jul 27, 2020

lidel left a comment •

edited

Loading

jessicaschilling commented Jul 29, 2020

aschmahmann left a comment

lidel commented Aug 19, 2020 •

edited

Loading

aschmahmann Aug 19, 2020

aschmahmann Aug 20, 2020 •

edited

Loading

lidel Aug 20, 2020 •

edited

Loading

aschmahmann Aug 20, 2020

[BOUNTY] Directory page UI improvements #7536

[BOUNTY] Directory page UI improvements #7536

Conversation

neatonk commented Jul 12, 2020 • edited by lidel Loading

This comment has been minimized.

jessicaschilling commented Jul 13, 2020

lidel left a comment

Choose a reason for hiding this comment

neatonk commented Jul 14, 2020

lidel left a comment • edited Loading

Choose a reason for hiding this comment

neatonk commented Jul 15, 2020

jessicaschilling commented Jul 21, 2020

lidel left a comment • edited Loading

Choose a reason for hiding this comment

lidel left a comment • edited by jessicaschilling Loading

Choose a reason for hiding this comment

jessicaschilling commented Jul 23, 2020 • edited Loading

jessicaschilling commented Jul 24, 2020

jessicaschilling commented Jul 24, 2020

neatonk commented Jul 26, 2020

neatonk commented Jul 26, 2020

neatonk commented Jul 27, 2020

jessicaschilling commented Jul 27, 2020

lidel commented Jul 27, 2020

lidel left a comment • edited Loading

Choose a reason for hiding this comment

jessicaschilling commented Jul 29, 2020

aschmahmann left a comment

Choose a reason for hiding this comment

lidel commented Aug 19, 2020 • edited Loading

aschmahmann Aug 19, 2020

Choose a reason for hiding this comment

aschmahmann Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

lidel Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

aschmahmann Aug 20, 2020

Choose a reason for hiding this comment

neatonk commented Jul 12, 2020 •

edited by lidel

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited by jessicaschilling

Loading

jessicaschilling commented Jul 23, 2020 •

edited

Loading

lidel left a comment •

edited

Loading

lidel commented Aug 19, 2020 •

edited

Loading

aschmahmann Aug 20, 2020 •

edited

Loading

lidel Aug 20, 2020 •

edited

Loading