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.

Sign up for GitHub

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

Jump to bottom

Added the inline pipeline #98

Merged
merged 6 commits into from
Feb 2, 2021
Merged

Added the inline pipeline #98

merged 6 commits into from
Feb 2, 2021

Conversation

DzenanJupic
Copy link
Contributor

@DzenanJupic DzenanJupic commented Dec 6, 2020 •
edited
Loading

A partial solution for #28. The main focus is on inlining raw HTML. But you can also inline CSS and JavaScript (without compression).

The new inline pipeline allows including the content of files directly into index.html. Just add <link data-trunk rel="inline" href="path/to/file"> to the position in your index.html where the content of the files should be placed, and you're good to go. There are three content types available: html, css, and js. html is pasted into index.html as is, css is wrapped in style tags and js is wrapped in script tags.
The type can be specified with the type attribute. If not specified, it's inferred from the file extension.

Example

/* path/to/file.css */
header {
    color: blue;
}
<!-- path/to/file.html -->
<header>LOGO</header>
// path/to/file.js
alert("Hello Trunk");
<!-- index.html -->
<html>
    <head>
        <title>Inline test</title>
        <link data-trunk rel="inline" href="/path/to/file.css">
    </head>
    <body>
        <link data-trunk rel="inline" href="/path/to/file.html">
        <!-- snip -->
        <link data-trunk rel="inline" href="/path/to/file.js">
    </body>
</html>

Will result in:

<!-- index.html -->
<html>
    <head>
        <title>Inline test</title>
        <style>
            header {
                color: blue;
            }
        </style>
    </head>
    <body>
        <header>LOGO</header>
        <!-- snip -->
        <script>alert("Hello Trunk");</script>
    </body>
</html>

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).

@DzenanJupic
Added the inline pipeline & updated CHANGELOG.md and README.md
634dbe3 
The inline pipeline allows including the content of files directly into `index.html`. Just add `<link data-trunk rel="inline" href="path/to/file">` to the position in your `index.html` where the content of the files should be placed, and you're good to go. There three content types available: `html`, `css`, and `js`. `html` is pasted into `index.html` as is, `css` is wrapped in `style` tags and `js` in wrapped in `script` tags.
The type can also be specified with the `type` attribute.
@thedodd
Copy link
Member

thedodd commented Dec 7, 2020

@DzenanJupic thanks for taking the time to knock this out! Let me review the implementation pattern here. I'll circle back with you shortly.

@DzenanJupic
Copy link
Contributor Author

@thedodd Sure.
Btw. It's an absolute pleasure to get started with this codebase! The code is really clean and well structured!

DzenanJupic
DzenanJupic commented Dec 9, 2020
View reviewed changes
src/pipelines/inline.rs Outdated Show resolved Hide resolved
DzenanJupic
DzenanJupic commented Dec 9, 2020
View reviewed changes
src/pipelines/inline.rs Outdated Show resolved Hide resolved
DzenanJupic
DzenanJupic commented Dec 9, 2020
View reviewed changes
src/pipelines/inline.rs Outdated Show resolved Hide resolved
@thedodd
Copy link
Member

thedodd commented Jan 23, 2021

@DzenanJupic hey there. Just circling back with you on this PR. Looks like there are clippy issues on CI. I have a PR I am about to merge which will hopefully address some of these. If the issues don't go away after merging my PR and updating this one, then it means that they are issues to address in your code :).

I'll keep you posted. Cheers

thedodd
thedodd requested changes Jan 25, 2021
View reviewed changes
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Dig it! Just a few tiny nits. Once those are done, we should be ready to merge and get this guy released!

src/pipelines/mod.rs Outdated Show resolved Hide resolved
src/pipelines/inline.rs Outdated Show resolved Hide resolved
src/pipelines/inline.rs Outdated Show resolved Hide resolved
src/pipelines/inline.rs Outdated Show resolved Hide resolved
src/pipelines/inline.rs Outdated Show resolved Hide resolved
src/pipelines/inline.rs Outdated Show resolved Hide resolved
@DzenanJupic
Copy link
Contributor Author

Hey @thedodd, I resolved all requested changes. Also, removing the clones was just a small refactor.

@DzenanJupic DzenanJupic requested a review from thedodd January 29, 2021 10:19
@thedodd thedodd mentioned this pull request Feb 1, 2021
Open
@thedodd
Copy link
Member

thedodd commented Feb 1, 2021 •
edited
Loading

@DzenanJupic hey boss. Merging a few other PRs has apparently caused conflicts here.

We can resolve these pretty easily. If you don't mind reverting your change to the Cargo.toml & the Cargo.lock (just git checkout master -- Cargo.toml and then the same thing for Cargo.lock), then there will be no conflicts to worry about. I am also in the process of updating the deps & the Trunk version over in #116 , so I'll take care of those items.

Once that is done, I should be able to merge this right away.

@thedodd thedodd mentioned this pull request Feb 1, 2021
Merged
@DzenanJupic
Copy link
Contributor Author

@thedodd I just reverted both Cargo.toml and Cargo.lock.
The only thing left for now is the CHANGELOG.md. It still says

## 0.8.0
### added
- Inline the content of files directly into `index.html` with the new `inline` asset. There are three content types that can be inlined: `html`, `css`, and `js`. The type can be specified with the `type` attribute or is inferred by the file extension.

Should I also revert this, or will you take care of it?

@thedodd
Copy link
Member

thedodd commented Feb 1, 2021

@DzenanJupic as long as there is no conflict in CHANGELOG.md, then we should be g2g. I think the line you added there is just fine.

@thedodd
Copy link
Member

thedodd commented Feb 1, 2021

Looks like perhaps the branch just needs to be updated with the latest code from master. Either a rebase or an update merge should do. Want to take care of that? Otherwise I don't mind just doing the update merge from the github UI.

@thedodd thedodd merged commit 24b7ab6 into trunk-rs:master Feb 2, 2021
@thedodd
Copy link
Member

thedodd commented Feb 2, 2021

Woot woot! You rock @DzenanJupic! Thanks for your contribution here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thedodd thedodd Awaiting requested review from thedodd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DzenanJupic @thedodd