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

[BOUNTY] Directory page UI improvements #7536

Merged
merged 5 commits into from
Aug 20, 2020
Merged

Conversation

neatonk
Copy link
Contributor

@neatonk neatonk commented Jul 12, 2020

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

@welcome

This comment has been minimized.

@jessicaschilling
Copy link
Contributor

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

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.

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

I tried:

  1. updated .gitmodule to point at your fork
  2. checked out version from [BOUNTY] Directory page UI improvements dir-index-html#38:
    $ cd assets
    $ git -C dir-index-html checkout c5d042ac5fd9c15631f95c6656e686a65f9f65ba
  3. regenerated assets via go generate .
  4. rebuilt go-ipfs
  5. 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
Copy link
Contributor Author

neatonk commented Jul 14, 2020

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

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.

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:

2020-07-14--15-55-38

To illustrate:

@neatonk
Copy link
Contributor Author

neatonk commented Jul 15, 2020

Thanks @lidel! I'll update the links.

@jessicaschilling
Copy link
Contributor

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

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.

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:

core/corehttp/gateway_indexPage.go Outdated Show resolved Hide resolved
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.

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

Next steps for the bounty:

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

@jessicaschilling
Copy link
Contributor

jessicaschilling commented Jul 23, 2020

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

neatonk commented Jul 26, 2020

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

@neatonk
Copy link
Contributor Author

neatonk commented Jul 26, 2020

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
Copy link
Contributor Author

neatonk commented Jul 27, 2020

@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
Copy link
Contributor

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
Copy link
Member

lidel commented Jul 27, 2020

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

lidel added a commit to neatonk/go-ipfs that referenced this pull request Jul 27, 2020
This adds subdomain tests for breadcrumbs added in:
ipfs#7536

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
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 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:

@lidel lidel mentioned this pull request Jul 27, 2020
72 tasks
@jessicaschilling
Copy link
Contributor

@neatonk - sent you a note re bounty!

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

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

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.

lidel added a commit to neatonk/go-ipfs that referenced this pull request Aug 19, 2020
This regenerates bindata as noted in
ipfs#7536 (review)

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
lidel added a commit to neatonk/go-ipfs that referenced this pull request Aug 19, 2020
This adds subdomain tests for breadcrumbs added in:
ipfs#7536

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel
Copy link
Member

lidel commented Aug 19, 2020

@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.

@lidel lidel requested a review from aschmahmann August 19, 2020 12:26
@@ -45,6 +45,7 @@ commit.
```bash
> go generate .
> git add bindata.go
> git add bindata_version_hash.go
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@aschmahmann aschmahmann Aug 20, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@lidel lidel Aug 20, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Kevin Neaton and others added 5 commits August 19, 2020 20:09
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
This adds subdomain tests for breadcrumbs added in:
ipfs#7536

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@aschmahmann 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
[BOUNTY] Directory page UI improvements

This commit was moved from ipfs/kubo@e80601b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/maintainers-input Needs input from the current maintainer(s) need/review Needs a review topic/gateway Topic gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants