Separate front-end components and add mock activities service. #2626

prodonjs · 2019-03-05T14:17:49Z

Adds deep-link routing behavior for all front-end pages.
Iframed pages reflect a URL to the browser that makes them
linkable but hides the internal page.
Seperates the front-end into subcomponents, including an
activities view.
Fix iframe view behavior to have it cover the space voided
by the layout header.
Adds /api/activities endpoint to the Express server for
activities to eventually be replaced with real data.
Sets the _devMode flag to true based on the webpack build env.

Addresses #2614 and #2615

This change is

prodonjs · 2019-03-05T14:18:21Z

/assign @avdaredevil

prodonjs · 2019-03-05T14:47:15Z

/test kubeflow-presubmit

avdaredevil · 2019-03-05T22:36:42Z

components/centraldashboard/app/server.ts

+    });
+}
+
+app.use(express.json());
 app.use(express.static(frontEnd));

 app.get("/api", (req: express.Request, res: express.Response) => {


Can we please seperate API's to ./api.ts and import it here as

import apiRoutes from './api' /* blah blah */ app.use(apiRoutes);

Understood. I intend to break this apart with the PR that addresses #2615 as it will require some additional changes in the API layer.

avdaredevil · 2019-03-05T22:38:53Z

components/centraldashboard/app/server.ts

+        });
+    }
+    return activities.sort((a, b) => {
+        return a.time > b.time ? -1 : 1;


Could technically also do:

activities.sort((a, b) => b.time - a.time)

Good suggestion. I like the succinctness.

avdaredevil · 2019-03-06T01:24:06Z

components/centraldashboard/public/.eslintrc.json

@@ -6,6 +6,7 @@
    "extends": ["eslint:recommended", "google"],
    "globals": {
        "VERSION": true,


Shouldn't version be a string perhaps "dev-build"?

Actually, this is just indicating to ESLint that the value is considered to be in-scope at runtime so the linter doesn't complain.

Based on the docs, it looks like the best option is to set them to readonly.

I see, thanks!

avdaredevil · 2019-03-06T01:25:58Z

components/centraldashboard/public/components/activity-view.js

+    _onResponse(responseEvent) {
+        const {status, response} = responseEvent.detail;
+        this.activities = [];
+        if (status === 200) {


Can we invert this condition and decrease code depth:

if (status != 200) return;

Good call. I think we'll want to add some kind of means for surfacing an request failure as well so that block will be expanded.

avdaredevil · 2019-03-06T01:29:11Z

components/centraldashboard/public/components/main-page.js

                    {
-                        link: 'https://www.kubeflow.org/docs/about/kubeflow/',
+                        iframeUrl: 'https://www.kubeflow.org/docs/about/kubeflow/',


Could we go with href? Not all are guaranteed to open in the iframe (ie. this one!)

I introduced href to be the attribute that is used when setting the anchor tag's href property on our UI to indicate the local "route". I changed link to iframeUrl since it was being assigned to the component's iframeUrl property. Other than the Home link which I moved out of the menuLinks list, all of the other ones opened in the iframe view in the existing code.

In your new PR please also move out the docs page, thanks!

avdaredevil · 2019-03-06T01:30:58Z

components/centraldashboard/public/components/main-page.js

                        text: 'Kubeflow docs',
+                        href: '/docs',


Let's call this route so it's clear this is a route for the current page rather than a Hypertext REFerence.

See my above comment for my rationale. I'm not opposed to this change but LMK what you think.

avdaredevil · 2019-03-06T01:34:35Z

components/centraldashboard/public/components/main-page.js

+        const url = new URL(e.currentTarget.href);
+        window.history.pushState({}, null, `_${url.pathname}`);
+        window.dispatchEvent(new CustomEvent('location-changed'));
+        e.preventDefault();


Does this actually work in the cluster? If it's a matter of unique routes, we can prefix all our routes with d/ for dashboard.

It does. I also considered the anchor links approach which can be configured in app-location. That's a more futureproof solution if we are concerned about collisions.

avdaredevil · 2019-03-06T01:35:33Z

components/centraldashboard/public/components/main-page.pug

+    app-location(route='{{route}}')
+    app-route(route='{{route}}', pattern='/:page', data='{{routeData}}', tail='{{subRouteData}}')
+
+


One too many extra whitespaces

avdaredevil · 2019-03-06T01:36:19Z

components/centraldashboard/public/components/main-page.pug

-            section.build build version 
-                span(title="Build: v[[buildVersion]] | Dashboard: v[[dashVersion]]") v[[buildVersion]]
+            section.build build version
+                span(title="Build: v[[buildVersion]] | Dashboard: v[[dashVersion]]")  v[[buildVersion]]


Revert change on this line

My editor is set to trim trailing whitespace which would be a common setting many users would have. I added the &nbsp to be explicit.

avdaredevil · 2019-03-06T01:38:55Z

components/centraldashboard/public/components/main-page.pug

+                        paper-tab(page='dashboard', link)
+                            a.link(tabindex='-1', href='/') DASHBOARD
+                        paper-tab(page='activity', link,hidden$='[[!_devMode]]')
+                            a.link(tabindex='-1', href='/activity') ACTIVITY


The case doesn't match the mock:

Also great job with simplifying the neon-animatable-pages with the app-router! 🎉

Thanks. I think I got caught up viewing the GCP console Dashboard and Activity tabs which use all caps.

With respect to the namespace selector, ours has never included the rounded border shown in the mock, but I didn't make any changes to it in this PR.

prodonjs · 2019-03-08T15:08:20Z

@avdaredevil PTAL when you can. This merges the changes from #2651. If possible, I'd like to get this change in since it separates the front-end components and makes it easier to divide work going forward.

prodonjs · 2019-03-08T15:45:02Z

Also, the responsiveness doesn't work particularly well on mobile if that's a concern at this point. This was true of the code that I merged from as well. I don't think we need to address it here but it shouldn't be too tough to give it a flex layout with wrapping.

avdaredevil · 2019-03-08T19:32:21Z

Responsiveness isn't a required feature (in fact it's not even in the pipeline as most of our clients are on Desktop / Laptops). However, I decided to make it slightly responsive for those screens for the sake of 800px to 1600px screens

avdaredevil · 2019-03-08T19:53:08Z

/lgtm
/approve

k8s-ci-robot · 2019-03-08T19:53:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avdaredevil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/centraldashboard/OWNERS~~ [avdaredevil]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

- Adds deep-link routing behavior for all front-end pages. Iframed pages reflect a URL to the browser that makes them linkable but hides the internal page. - Seperates the front-end into subcomponents, including an activities view. - Fix iframe view behavior to have it cover the space voided by the layout header. - Adds /api/activities endpoint to the Express server for activities to eventually be replaced with real data. - Sets the _devMode flag to true based on the webpack build env.

prodonjs · 2019-03-08T20:33:39Z

@avdaredevil Merge conflict fixed. PTAL.

avdaredevil · 2019-03-08T21:12:04Z

/lgtm

…low#2626) * Separate front-end components and add mock activities service. - Adds deep-link routing behavior for all front-end pages. Iframed pages reflect a URL to the browser that makes them linkable but hides the internal page. - Seperates the front-end into subcomponents, including an activities view. - Fix iframe view behavior to have it cover the space voided by the layout header. - Adds /api/activities endpoint to the Express server for activities to eventually be replaced with real data. - Sets the _devMode flag to true based on the webpack build env. * Address PR comments * Merge changes from PR kubeflow#2651

googlebot added the cla: yes label Mar 5, 2019

k8s-ci-robot requested review from avdaredevil and ellistarn March 5, 2019 14:17

k8s-ci-robot added the size/XL label Mar 5, 2019

k8s-ci-robot assigned avdaredevil Mar 5, 2019

avdaredevil suggested changes Mar 6, 2019

View reviewed changes

prodonjs force-pushed the routing-and-activities branch from 9896909 to 7b3fad3 Compare March 8, 2019 14:56

prodonjs force-pushed the routing-and-activities branch from 7b3fad3 to b6c8df8 Compare March 8, 2019 16:28

k8s-ci-robot added the lgtm label Mar 8, 2019

k8s-ci-robot added the approved label Mar 8, 2019

prodonjs added 3 commits March 8, 2019 15:31

Address PR comments

01c4dab

Merge changes from PR kubeflow#2651

4ea2c5b

prodonjs force-pushed the routing-and-activities branch from b6c8df8 to 4ea2c5b Compare March 8, 2019 20:31

k8s-ci-robot removed the lgtm label Mar 8, 2019

k8s-ci-robot added the lgtm label Mar 8, 2019

k8s-ci-robot merged commit 3f4f39f into kubeflow:master Mar 8, 2019

prodonjs mentioned this pull request Mar 11, 2019

[Dashboard v1] Support deep-links #2614

Closed

prodonjs deleted the routing-and-activities branch March 19, 2019 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate front-end components and add mock activities service. #2626

Separate front-end components and add mock activities service. #2626

prodonjs commented Mar 5, 2019 •

edited by jlewi

Loading

prodonjs commented Mar 5, 2019

prodonjs commented Mar 5, 2019

avdaredevil Mar 5, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 5, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 8, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 8, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

avdaredevil Mar 6, 2019

prodonjs Mar 6, 2019

prodonjs commented Mar 8, 2019

prodonjs commented Mar 8, 2019

avdaredevil commented Mar 8, 2019

avdaredevil commented Mar 8, 2019

k8s-ci-robot commented Mar 8, 2019

prodonjs commented Mar 8, 2019

avdaredevil commented Mar 8, 2019

		app-location(route='{{route}}')
		app-route(route='{{route}}', pattern='/:page', data='{{routeData}}', tail='{{subRouteData}}')

Separate front-end components and add mock activities service. #2626

Separate front-end components and add mock activities service. #2626

Conversation

prodonjs commented Mar 5, 2019 • edited by jlewi Loading

prodonjs commented Mar 5, 2019

prodonjs commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prodonjs commented Mar 8, 2019

prodonjs commented Mar 8, 2019

avdaredevil commented Mar 8, 2019

avdaredevil commented Mar 8, 2019

k8s-ci-robot commented Mar 8, 2019

prodonjs commented Mar 8, 2019

avdaredevil commented Mar 8, 2019

prodonjs commented Mar 5, 2019 •

edited by jlewi

Loading