feat: add a default home page #16

raymondfeng · 2018-09-24T15:51:29Z

Add a default home page for the app. We'll use it as a template for loopbackio/loopback-next#1745.

We need to fix @loopback/rest so that it respects the correct content-type from responses. At the moment, the home page will render raw html as the media type is plain/text for string response from the controller method.

virkt25 · 2018-09-24T16:08:49Z

I made the following changes to get the page to render :D

Render:

Looks good overall -- have some concerns with the resources we're loading -- will leave a comment.

virkt25 · 2018-09-24T16:11:41Z

src/controllers/home-page.controller.ts

+  <meta name="viewport" content="width=device-width, initial-scale=1">
+
+  <!-- Latest compiled and minified CSS -->
+  <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u"


Can we skip out on loading the full bootstrap.min.css and the accompanying js file? We're using about 2 styles from this entire file. I'd rather we copy the style in-lined for a faster render. No JS is needed. (Currently this + JS is loading 63.1 KB while the page payload is only 1.7 KB).

An alternative IIRC is that BootStrap also provides just the grid CSS -- which is lighter and includes the CSS for container class.

virkt25 · 2018-09-24T16:12:09Z

src/controllers/home-page.controller.ts

+  </footer>
+  <!-- Bootstrap core JavaScript -->
+  <!-- Placed at the end of the document so the pages load faster -->
+  <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.12.4/jquery.min.js"></script>


Nothing on this page needs the JS ... no BootStrap widgets are used.

virkt25 · 2018-09-24T16:12:50Z

src/controllers/home-page.controller.ts

+  <div class="container">
+    <h1>${this.name}@${this.version}</h1>
+    <ul>
+      <li><a href="/openapi.json">OpenAPI spec: /openapi.json</a></li>


Can we style the list to be bigger. Plus can OpenAPI spec: not be part of the link. Bolded. Would be a better style imo.

Same applies for the bullet below.

virkt25 · 2018-09-24T16:14:11Z

src/controllers/home-page.controller.ts

+  </div>
+  <footer>
+    <div class="container">
+      <p>


Let's center this text on the page.

raymondfeng · 2018-09-24T16:56:35Z

@virkt25 You worked around the issue by manipulating Express response directly. In this case, we should introduce a special return value such as ByPassResponse so that our writer won't try to send 204.

Feel free to improve the UI. I'm a beginner there :-).

virkt25 · 2018-09-24T17:00:39Z

You worked around the issue by manipulating Express response directly. In this case, we should introduce a special return value such as ByPassResponse so that our writer won't try to send 204.

I think a simpler fix can even be just to check if Content-Type header has been set in the writer ... and if so we don't overwrite it -- and then we can rely on the response.end in the write.

And of course in the long term it should request the responses decorator + the expected content type header.

Feel free to improve the UI. I'm a beginner there :-).

I'll push some changes to the PR in a bit :)

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng · 2018-09-24T21:39:33Z

Please note that the CI failure is not related to this change. It will be fixed as part of #14.

virkt25 · 2018-09-24T23:16:51Z

Here's a screenshot for the UI -- no external CSS / JS. Minor variances with some alternatives below. I personally vote for Alternative 1 (I tried it after pushing my changes otherwise I would've included that in the PR). Anyone else have any preferences? cc: @raymondfeng @bajtos @strongloop/sq-lb-apex

Current:

Alternative 1:

Alternative 2:

Alternative 3:

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

bajtos · 2018-09-25T08:55:52Z

I vote for Alternative 1.

Perhaps instead of using name@version format, can we move the version number to line 2, to make it more explicit what is version and what is the app name?

A mock-up:

@loopback/example-shopping
version 1.0.0

bajtos · 2018-09-25T09:00:04Z

public/index.html.template

+<body>
+  <h1 style="text-align: center">${name}@${version}</h1>
+  <h3>OpenAPI spec: <a href="/openapi.json">/openapi.json</a></h4>
+  <h3>API Explorer: <a href="/explorer">/explorer</a></h4>


IIRC, applications can customize the URL of both OpenAPI spec and Explorer. See loopbackio/loopback-next#1637, Customize How OpenAPI Spec is Served and Configure the API Explorer.

IMOL, the home page should respect these settings. Eventually :) I am fine to keep the current static version as the first baby step.

bajtos · 2018-09-25T09:00:54Z

src/controllers/home-page.controller.ts

+  description: string;
+  render: TemplateExecutor;
+
+  constructor(@inject(RestBindings.Http.RESPONSE) private res: Response) {


Can we use response instead of res please?

bajtos · 2018-09-25T10:34:45Z

src/controllers/home-page.controller.ts

+import {inject} from '@loopback/context';
+import {RestBindings, Response} from '@loopback/rest';
+
+const pkg = require('../../../package.json');


From the perspective of clean design and Dependency Injection pattern, I think controller should not depend package.json directly, but get it injected from app context. Then the application should bind the content of package.json (maybe a subset of properties only?) in its constructor.

Not a big deal for this particular pull request, but I think we should think more about the design patterns when we start working on adding HomePageController to our project template.

Yes, I'm thinking about the same direction while I work on this implementation.

Signed-off-by: Raymond Feng <[email protected]>

Signed-off-by: virkt25 <[email protected]>

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng requested review from bajtos and virkt25 September 24, 2018 15:51

raymondfeng force-pushed the home-page branch from f37bc9f to 0f3779c Compare September 24, 2018 15:52

virkt25 reviewed Sep 24, 2018

View reviewed changes

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

7af3258

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng mentioned this pull request Sep 24, 2018

[WIP] feat(rest): allow bypassing http response writing and custom content type loopbackio/loopback-next#1753

Closed

7 tasks

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

d986a61

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng mentioned this pull request Sep 24, 2018

Scaffolded LB4 app reports 404 for the home page loopbackio/loopback-next#1745

Closed

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

e8bbf7b

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng force-pushed the home-page branch 2 times, most recently from 4667561 to 2258e98 Compare September 24, 2018 18:28

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

61099ca

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

df2a5fa

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

21c4f41

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 24, 2018

feat(rest): allow bypassing http response writing and custom content …

b4808b0

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

raymondfeng force-pushed the home-page branch 2 times, most recently from f1cc2bb to 23da6cb Compare September 24, 2018 22:34

raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Sep 25, 2018

feat(rest): allow bypassing http response writing and custom content …

deb2f05

…type We discover an issue to return html from a controller: See loopbackio/loopback4-example-shopping#16

bajtos approved these changes Sep 25, 2018

View reviewed changes

bajtos reviewed Sep 25, 2018

View reviewed changes

raymondfeng added 3 commits September 25, 2018 08:07

feat: add a default home page

d00cd44

Signed-off-by: Raymond Feng <[email protected]>

chore: move html template to /public

0a5a046

Signed-off-by: Raymond Feng <[email protected]>

fix: add response object for ping

e48f215

Signed-off-by: Raymond Feng <[email protected]>

virkt25 and others added 2 commits September 25, 2018 08:09

feat: update index.html design

7197f17

Signed-off-by: virkt25 <[email protected]>

chore: clean up ping controller

af2db9d

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng force-pushed the home-page branch 2 times, most recently from 7cfc221 to 226347d Compare September 25, 2018 15:10

fix: improve home page controller with di of pkg

f0c76eb

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng force-pushed the home-page branch 2 times, most recently from 366433c to 5334669 Compare September 25, 2018 18:09

chore: simplify test cases

df3c287

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng force-pushed the home-page branch from 5334669 to df3c287 Compare September 25, 2018 18:11

raymondfeng mentioned this pull request Sep 25, 2018

user order history component #12

Merged

raymondfeng merged commit 293177b into master Sep 25, 2018

raymondfeng deleted the home-page branch September 25, 2018 18:19

raymondfeng mentioned this pull request Sep 26, 2018

feat(boot): bind content of package.json to app context loopbackio/loopback-next#1764

Merged

7 tasks

bajtos mentioned this pull request Oct 1, 2018

feat(cli): add code template for default home page controller loopbackio/loopback-next#1763

Merged

7 tasks

bajtos mentioned this pull request Mar 19, 2019

File downloads (streaming responses) loopbackio/loopback-next#2230

Closed

feat: add a default home page #16

feat: add a default home page #16

Uh oh!

Conversation

raymondfeng commented Sep 24, 2018

Uh oh!

virkt25 commented Sep 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Sep 24, 2018

Uh oh!

virkt25 commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raymondfeng commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

virkt25 commented Sep 24, 2018

Current:

Alternative 1:

Alternative 2:

Alternative 3:

Uh oh!

bajtos commented Sep 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

virkt25 commented Sep 24, 2018 •

edited

Loading

raymondfeng commented Sep 24, 2018 •

edited

Loading