Ms app page #290

msecret · 2016-05-20T20:37:45Z

Adds an app page with basic information as defined in #220. Uses the app's summary API route for app name, state, last push, buildpack. Uses app's stat API route for current mem/disk vs limit.

Picked the simplest UI possible, by showing this as a horizontal table. This was done to limit having to redo work when we learn more from design.

coveralls · 2016-05-20T23:14:18Z

Coverage remained the same at 72.599% when pulling 1d2159b8ac2986181d365bbe44882ecdaf644f59 on ms-app_page into d35b788 on staging-alpha.

coveralls · 2016-05-20T23:30:02Z

Coverage remained the same at 72.599% when pulling 16eff12a26be91001a1cf5e89257cb7cc563052b on ms-app_page into d35b788 on staging-alpha.

msecret · 2016-05-23T18:16:02Z

static_src/app.jsx

@@ -81,7 +81,7 @@ export default class App extends React.Component {
  }
 }
 App.propTypes = {
-  children: React.PropTypes.element,
+  children: React.PropTypes.any,


This was getting rid of an annoying error because I think we pass different things as children, sometimes an arry of elements, sometimes maybe just an element?

msecret · 2016-05-24T23:55:58Z

This is what this looks like:

@mogul one thing to point out: I'm not using the UI design on the current app page of those little boxes with different information. This is because we'd have to code it anew, we don't have a full screen page like the current deck does anymore and we don't know if it'd be a successful UI as we don't have research behind it.

If you feel that we should layout this page in the same manner as the current deck app page so that our current user's will understand where things are, let us know and we can do that.

jeremiak · 2016-05-25T20:16:24Z

static_src/components/app_page.jsx

+    this._onChange = this._onChange.bind(this);
+  }
+
+  componentDidMount() {


Make sure to clean up any listeners on componentWillUnmount()

jeremiak · 2016-05-25T20:30:09Z

Hey @msecret this looks pretty great, nice work.

I think it should be merged after #256 and the AppStore internals should be updated to use the new methods like .push() and .merge(). Other than that I think it works well for now while we're doing research on what data users need to see here.

Also way to kick that weird app-URL-has-funky-spaces bug with dedent!

msecret · 2016-05-25T20:35:29Z

That sounds fine, we can work on getting #256 merged

mogul · 2016-05-25T21:37:53Z

I could opine, but I think I'd rather defer to @vz3 and @juliaelman on this...

I'm not using the UI design on the current app page of those little boxes with different information. This is because we'd have to code it anew, we don't have a full screen page like the current deck does anymore and we don't know if it'd be a successful UI as we don't have research behind it.

If you feel that we should layout this page in the same manner as the current deck app page so that our current user's will understand where things are, let us know and we can do that.

Victor and Julia, this is a read-only interim-rewrite of the top of the existing app page, which looks like this...

It's happening because we're trying to do the minimum work of reimplementation necessary to move off of the old code base to the new one. Do you think this is acceptable? If not, are there simple changes you'd bless us doing to make the step down acceptable until we have user-research driving the redesign of this page?

msecret · 2016-05-25T22:25:53Z

@thisisdano take a look at Bret's last comment too.

juliaelman · 2016-05-26T15:25:55Z

I am going to defer to @thisisdano on this, but think that it might be good for us to start thinking about better styling for tables for cloud.gov overall. It's something we're working on for Compliance Viewer too.

thisisdano · 2016-05-26T19:14:43Z

That new content section/table is ugly af, but a few fixes would improve it a bunch.

Lack of padding in the main content container makes everything worse — 12-16px of padding is a necessary start.
"About" subhed feels redundant since there are no other options/tabs.
Table itself has too much padding and has unnecessary right/left borders. Leave the borders on the top and bottom of each row, remove the right/left borders on each cell, reduce the padding by 50% on the top/bottom of each cell, and remove the padding from the left of each cell.
Memory and Disk Limit are a little difficult to parse as is. They appear to be paired values [n]/[N], but the relationship isn't clear. Is it saying the app is consuming [n] out of [N] total available? Maybe reformat display to read <b>[n]</b> out of [N] ([p]%)
Is Disk Limit the same as (what seems to be) Disk usage in the Live section of the earlier screen? Should the title of the row be Disk Usage?
Together, those changes would immediately improve the legibility of the content and could be an acceptable interim solution

I had to change the app commitmponent to app page because theres already a app committedmponet for the mainin committedmonent.

TODO - shrinkwrap - any tests - maybe getting additional data sources for app page

So in the [npm shrinkwrap docs](https://docs.npmjs.com/cli/shrinkwrap), it says running `npm install` will update the shrinkwrap file, but it does not. This is probably because we're on an old npm version.

The stats will get added to the main app. I also added npm-debug.log to git ignore because it pops up sometimes when something goes wrong with an npm script, and shouldn't be checked in.

Actually passed a string of elements to app.

The cf api stats app route doesn't give the app guid back, so it has to be passed back to resv so it's in the data.

I can't think of what to do if the stats are there for an app that isn't there yet, because the data would be uncomplete, so just throwing an error. Just though of a problem: what if the app stats request gets in before the app summary request? Should probably change how the requests are done.

Rather then throwing an error when the app for the stats doesn't exist yet, this creates a new app, to ensure it will still work if the app stats request comes in before the app summary one.

The cfi api method for app stats returns a weird object with numbers as keys, so rather then using shared `fetch` method, used custom ajax method and return the first element in weird object.

Formatting the bytes format it comes in as megabytes and showing both current usage and limit.

This is what we're supposed to do.

coveralls · 2016-05-26T22:19:14Z

Coverage remained the same at 72.599% when pulling e26b040 on ms-app_page into 23d64a9 on staging-alpha.

Have the app store use the new methods for immutable.

msecret · 2016-05-26T22:56:24Z

I talked with @thisisdano on slack and we decided that we shouldn't focus on wider design issues here and that a table is fine UI for now.

jeremiak · 2016-05-27T20:35:23Z

Looks good to me, nice work on the code. Merging now for #295 and for my upcoming work in #289

msecret added the review-ready label May 23, 2016

msecret reviewed May 23, 2016
View reviewed changes

jeremiak reviewed May 25, 2016
View reviewed changes

jeremiak mentioned this pull request May 26, 2016

Add view of events and logs in app page #289

Closed

2 tasks

Marco Segreto and others added 13 commits May 26, 2016 15:06

Start of page.

802c06a

Add store and actions for app page

ba1bcc8

I had to change the app commitmponent to app page because theres already a app committedmponet for the mainin committedmonent.

Finished actual page

3294369

TODO - shrinkwrap - any tests - maybe getting additional data sources for app page

Update shrinkwrap file

6038125

So in the [npm shrinkwrap docs](https://docs.npmjs.com/cli/shrinkwrap), it says running `npm install` will update the shrinkwrap file, but it does not. This is probably because we're on an old npm version.

Api and actions for app stats

f300fbc

The stats will get added to the main app. I also added npm-debug.log to git ignore because it pops up sometimes when something goes wrong with an npm script, and shouldn't be checked in.

Add app stat fetching to app route

6169ba7

Fix app error

448c029

Actually passed a string of elements to app.

Add app guid to recv app stats

b7c5dd8

The cf api stats app route doesn't give the app guid back, so it has to be passed back to resv so it's in the data.

Fix lint errors for newly touched files

39277be

Create new app on app stats resv

839bd81

Rather then throwing an error when the app for the stats doesn't exist yet, this creates a new app, to ensure it will still work if the app stats request comes in before the app summary one.

Added app store stats fetch case

fb2bad7

Changed the cf api for weird data format

4d90e69

The cfi api method for app stats returns a weird object with numbers as keys, so rather then using shared `fetch` method, used custom ajax method and return the first element in weird object.

Marco and others added 3 commits May 26, 2016 15:08

App page mem and disk display.

3584f39

Formatting the bytes format it comes in as megabytes and showing both current usage and limit.

Redid shrinkwrap with dev deps

145603a

This is what we're supposed to do.

Attempt to fix npm shrinkwrap merge conflict

790695b

msecret force-pushed the ms-app_page branch from 16eff12 to 790695b Compare May 26, 2016 22:10

Another try

e26b040

Merge in store changes

0551aa1

Have the app store use the new methods for immutable.

msecret mentioned this pull request May 27, 2016

Ms routes app page #295

Merged

jeremiak merged commit 736e813 into staging-alpha May 27, 2016

jeremiak deleted the ms-app_page branch May 27, 2016 20:35

jeremiak removed the review-ready label Jun 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ms app page #290

Ms app page #290

msecret commented May 20, 2016 •

edited

Loading

coveralls commented May 20, 2016

coveralls commented May 20, 2016

msecret May 23, 2016

msecret commented May 24, 2016

jeremiak May 25, 2016

jeremiak commented May 25, 2016

msecret commented May 25, 2016

mogul commented May 25, 2016

msecret commented May 25, 2016

juliaelman commented May 26, 2016

thisisdano commented May 26, 2016 •

edited

Loading

coveralls commented May 26, 2016

msecret commented May 26, 2016

jeremiak commented May 27, 2016

Ms app page #290

Ms app page #290

Conversation

msecret commented May 20, 2016 • edited Loading

coveralls commented May 20, 2016

coveralls commented May 20, 2016

msecret May 23, 2016

Choose a reason for hiding this comment

msecret commented May 24, 2016

jeremiak May 25, 2016

Choose a reason for hiding this comment

jeremiak commented May 25, 2016

msecret commented May 25, 2016

mogul commented May 25, 2016

msecret commented May 25, 2016

juliaelman commented May 26, 2016

thisisdano commented May 26, 2016 • edited Loading

coveralls commented May 26, 2016

msecret commented May 26, 2016

jeremiak commented May 27, 2016

msecret commented May 20, 2016 •

edited

Loading

thisisdano commented May 26, 2016 •

edited

Loading