Skip to content

feat(up): checkout and use helix-pages if no source scripts are present#946

Merged
tripodsan merged 2 commits intomasterfrom
use-helix-pages
Jun 6, 2019
Merged

feat(up): checkout and use helix-pages if no source scripts are present#946
tripodsan merged 2 commits intomasterfrom
use-helix-pages

Conversation

@tripodsan
Copy link
Contributor

@tripodsan tripodsan commented Jun 5, 2019

fixes #929

2019-06-05_17-34-53

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #946 into master will increase coverage by 0.16%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   91.55%   91.71%   +0.16%     
==========================================
  Files          45       46       +1     
  Lines        1848     1908      +60     
==========================================
+ Hits         1692     1750      +58     
- Misses        156      158       +2
Impacted Files Coverage Δ
src/up.cmd.js 87.66% <100%> (+2.62%) ⬆️
src/helix-pages.js 93.93% <93.93%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d95b79...3264c9b. Read the comment docs.

Copy link
Contributor

@kptdobe kptdobe left a comment

Choose a reason for hiding this comment

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

hummmm... why copying the whole helix-pages project as is ? I think this is weird...
Why dumping scrani.js especially if you do nothing with it...
I would simply create a default basic html.htl, html.pre.js and minimal style.css since this is the only thing you want to test for now.

@tripodsan
Copy link
Contributor Author

hummmm... why copying the whole helix-pages project as is ? I think this is weird...

do you mean in the tests? I want to be able to run the test offline:

// init helix-pages fake local repo. we do this, because we don't want git checkouts in
// the helix-cli repository, and we want to keep the tests offline
await fse.copy(path.resolve(__dirname, 'fixtures', 'helix-pages', 'master'), helixPagesRepo);
initGit(helixPagesRepo, 'https://github.com/adobe/helix-pages.git');

Why dumping scrani.js especially if you do nothing with it...

well, it's part of helix-pages at the moment.

I would simply create a default basic html.htl, html.pre.js and minimal style.css since this is the only thing you want to test for now.

ok. I remove it.

@tripodsan tripodsan requested a review from kptdobe June 5, 2019 23:46
Copy link
Contributor

@kptdobe kptdobe left a comment

Choose a reason for hiding this comment

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

I think now it is worst... the code still contains scrani references in multiple places.
My point is more to the fact that we do not need so much code in this context. Your test only asserts that when there is no code in the project, some default code is provided and some rendering is executed. The "default code" provided does not need to be a copy of the current helix-pages project and could / should be something really basic to limit the amount of lines of code we introduce in the cli project. Really, this super long style.css do not bring any value here. Same for html_plain.htl, html_planing.pre.js...

<meta name="viewport" content="width=device-width, initial-scale=1"/>
<link rel="shortcut icon" type="image/x-icon" href="/favicon.ico">
<script src="https://d6uhzlpot4xwe.cloudfront.net/runtime/1.21/themes/wiper-fonts.gz.js"></script>
<script src="/scrani.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<script src="/scrani.js"></script>

<script src="/scrani.js"></script>
<script>
window.onload = function() {
scrani.onload();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
scrani.onload();

@tripodsan
Copy link
Contributor Author

I think now it is worst...

ok ok ....

@tripodsan
Copy link
Contributor Author

@kptdobe I initially thought, that we could add more and more tests, to see if the html is rendered correctly. but now I think this should be done in the example project instead. but you're right. I remove the extra files.

@tripodsan tripodsan merged commit 066e90b into master Jun 6, 2019
@tripodsan tripodsan deleted the use-helix-pages branch June 6, 2019 06:31
trieloff pushed a commit that referenced this pull request Jun 6, 2019
# [4.1.0](v4.0.9...v4.1.0) (2019-06-06)

### Features

* **up:** checkout and use helix-pages if no source scripts are present ([#946](#946)) ([066e90b](066e90b)), closes [#929](#929)
@adobe-bot
Copy link
Collaborator

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hlx up without config should render with helix-pages code

4 participants