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

feat(cli): add code template for default home page controller #1763

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Sep 26, 2018

This PR adds code template for a default home page controller to be generated as part of lb4 app.

Fixes #1745

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng raymondfeng requested a review from bajtos as a code owner September 26, 2018 00:28
Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Yay for these additions and especially the test setup cleanup. Much simpler / easier flow. Just some minor comments.

'src/controllers/home-page.controller.ts',
/homePage\(\)/,
);
assert.fileContent(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also assert ./test-helper.ts file

Client,
} from '@loopback/testlab';

export interface AppWithClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to be below the function?


<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Update to the newest design so we have a favicon (otherwise loading this throws a console error in the browser (if someone was to check)).

@raymondfeng
Copy link
Contributor Author

@virkt25 Updated. PTAL.

@virkt25
Copy link
Contributor

virkt25 commented Sep 26, 2018

Once this lands -- we will need a follow up PR to add this homepage to our existing examples under examples/*

@raymondfeng raymondfeng merged commit f4be330 into master Sep 28, 2018
@raymondfeng raymondfeng deleted the add-default-home-page branch September 28, 2018 18:57
<body>
<div class="info">
<h1><%= project.name %></h1>
<p>Version <%= project.version || '1.0.0' %></p>
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, we are hard-coding project name and version to index.html at the time when the HomePageController is scaffolded. Subsequent updates to name and version in project's package.json files are not going to be picked up, the home page will display version 1.0.0 forever. I find that rather confusing!

@raymondfeng @virkt25 Is this intentional? I thought our intention was to render the home pages from an EJS template at runtime, that's what I see in loopbackio/loopback4-example-shopping#16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional as the first step:

We wanted to keep the HomePageController implementation simple and easy to understand.

  • The generated public/index.html is a plain html instead of a template. We set the name/version during code gen but it's up to developers to maintain it
  • There is no need to use a template engine to render the view

That's been said, I'm fine to upgrade it to an EJS view once #1764 is landed.

Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the Version from the static page then? Application name is unlikely to change, but the version is going to change often.

@b-admike b-admike mentioned this pull request Oct 4, 2018
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants