-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
@lidel and @ribasushi, could you please weigh in? If you're OK with this approach, we may want to consider using a release based on this in ipfs/kubo#7536, for efficiency. Thank you! |
@jessicaschilling I think this is great! One procedural nit: do we care how heavy the page is? If we do, perhaps we can use a real minifier in place of the basic Note - if we do not care much about the pagesize and/or the source is already "pretty tight" - this comment should probably be disregarded. A few stylistic nits remain, but if you decide to use a smarter minifier they are moot anyway. |
@ribasushi Yep, the idea here was to keep the whole thing as lightweight as possible. We aren't using any other modules, so adding one just for this purpose didn't seem like the ideal way to go. If you've got ideas for selectively removing spaces in sed etc while preserving what's needed in css, that'd be great. |
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.
LGTM, I would add an extra newline for readability ( diff in comment )
Co-authored-by: Peter Rabbitson <ribasushi@protocol.ai>
Thanks, @ribasushi -- over to you, @lidel, for any thoughts 😊 |
The file was moved to dist in: ipfs/dir-index-html#40 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
The file was moved to dist/ in: ipfs/dir-index-html#40 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
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.
I just realized dist/
is a breaking change because HTML file used by go-ipfs moved from dir-index-html/dir-index.html
to dir-index-html/dist/dir-index.html
, so when we switch go-ipfs to this, go-ipfs/assets/bindata.go
and assets.go
need to be changed.
To have a better understanding of the scope of changes I created ipfs/kubo#7561 – not terrible, but feels like unnecessary floating of the go-ipfs boat.
The main purpose of dir-index-html/dist/dir-index.html
was to ensure output file won't be edited directly. I think we could keep old location dir-index-html/dir-index.html
and add GithubAction that runs the build on PRs and if there are any uncommited changes - fail the build (I want to add it abyway – example)
Thoughts?
@lidel Sounds like a good plan. Updated to remove |
This adds GithubAction that runs the build and checks for any unstaged changes. This ensures `src/dir-index.html` match the output `dir-index.html` in the main directory.
6f04a55
to
8a9c62e
Compare
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.
LGTM.
-
From the point of view of go-ipfs there are no functional changes (we output to the same file), but this repo is now much easier to maintain and contribute to.
-
To ensure sources and minified output do not diverge again I've added Github Action in 8a9c62e; should be enough to guard against committing state where output does not match sources:
-
Just to be sure there are no regressions I tested against @neatonk's PR and works as expected.
@jessicaschilling feel free to merge
In this PR
src
directoryAdds newdist
directorybuild
script inpackage.json
to do the following:dist
directorydist/dir-index.html
that includes a minified, inlined version of external CSS files fromsrc
directory (and removes their corresponding<link>
s to stylesheets)test
directory at the builtdist/dir-index.html
Important!Closes #6, BUT note that this will require updating go-ipfs itself to look for the new file names. (@ribasushi and/or @lidel, is this something you're willing to address in a separate PR?)(we kept the same filename, no changes to go-ipfs needed)