-
Notifications
You must be signed in to change notification settings - Fork 55
[BOUNTY] Directory page UI improvements #38
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Thanks, @neatonk! This looks good to me but I'd like to have a closer review by the go-ipfs core team. @aschmahmann -- do you have the time, or can you suggest another reviewer? |
@aschmahmann -- just to note, this behaves as intended (meets all the criteria in #37 (comment) ) using the test server described here. I'm afraid I can't explicitly confirm/deny that the test info gives us enough confidence to merge. Is this something that you or @Stebalien can investigate? Apologies for the inconvenience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in #38 (comment) -- this meets all the requirements using the test server, but I still would like @aschmahmann or @Stebalien to confirm that we're confident to merge.
It wasn’t in the original bounty spec - however, @neatonk, is that something you could take on? We may be able to adjust the bounty as a result. |
The presence of additional content in the table was causing the width of the type-icon column to be reduced. Applying the width to the column and it's contents prevents this from happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick note -- can you please make the changes you made in dir-index.html in dir-index-uncat.html as well? Thank!
@jessicaschilling Is dir-index-uncat.html actually in use? If not, I would prefer to leave it alone. It is not clear how the changes I made should be applied and for what purpose. |
They are both in use, unfortunately. https://github.com/ipfs/dir-index-html#updating does include guidance to touch them both when updating, but this also could have been called out more explicitly in the issue; that's my fault. |
No worries and thanks for pointing that out. However, it looks to me like the two files are already out of sync. That will make it difficult for me to update Below are two possible solutions to this problem. I am in favor of Option B since it would be a better use of time and would not delay this PR. Option A First, tap someone who is familiar with previous changes to Option B Finalize this PR (and bounty) without updating |
Option B is fine. I'll need to rationalize the two files at a later date before we can release this, but if you're able to get everything working and tested for dir-index.html that's a start. Agreed that automating as in #6 is the best long-term solution, but we've not got anyone with the bandwidth for that at present. |
This is necessary to support subdomain resolution. E.g. {cid}.ipfs.localhost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 let's continue in ipfs/kubo#7536
Also branch |
This PR implements the changes described in #37.
This is an incremental improvement over the previous version of the
dir-index.html
page and I suggest that a larger restructuring of the page should be considered after this PR is completed. Additional changes informed by a discussion of design constraints (e.g. file size and responsiveness) and developer workflow (e.g. build tools and minification) would lead to greater improvements.Closes #37