Implement download button feature for logs #77

TheRealJon · 2018-05-29T17:03:24Z

Description

Log content can be downloaded via a download button on the top right corner of the log window. This downloads the currently displayed content (up to the last 1000 lines of the log).

Changes

Refactored co-m-pane__top-controls to top-controls and now uses flexbox.
- top-controls__group and modifiers top-controls__group--right, and --left can now be used as children to distribute control item groups across the top controls element. Items within these groups are aligned as the class name suggests (center by default). This is a wrapper for a group of control items in the top control element.
- top-controls__item should be used as a child of a top controls group element and are automatically vertically centered, and spaced 15px apart horizontally. This is a wrapper for each control item in a top controls group element.
Implemented download functionality in the ResourceLog component
Removed obsolete _resource-log.scss. Top controls style refactoring made these styles unnecessary.
Update PodExec component to use new top controls styles (only other place where top controls was used).

Screenshots

320px screen width

@openshift/team-ux-review

robszumski · 2018-05-29T17:12:50Z

Can we title this button in verb + noun format?

Download logs, Download history, or similar? Thoughts @openshift/team-ux-review?

TheRealJon · 2018-05-29T17:20:27Z

@robszumski I thought the same, but went ahead with just 'Download' since that is what is used on the YAML editor tab. If we do use verb+noun format here, we might want to revisit other places where a download feature is provided and make sure we are consistent across the board.

spadgett · 2018-05-29T17:28:55Z

@TheRealJon Is any reflow needed at mobile?

@rhamilto PTAL

TheRealJon · 2018-05-29T17:44:22Z

@spadgett I don't think any reflow is needed. I'm not sure what screen widths we need to account for, but this layout works all the way down to ~450px.

rhamilto · 2018-05-29T18:09:50Z

frontend/public/components/pod-exec.jsx

-          Connecting to
-        </span>
-        <Dropdown className="btn-group" items={_.mapValues(containers, nameWithIcon)} title={nameWithIcon(activeContainer || <LoadingInline />)} onChange={this.onChangeContainer} />
+        <div className="co-m-pane__top-controls__group--left">


BEM suggests you shouldn't nest elements. http://getbem.com/faq/#css-nested-elements

I would like to remove the co-m-pane from this and just have the top-controls be the block level class. Then we can just have top-controls, top-controls__group, top-controls__item.

rhamilto · 2018-05-29T18:10:25Z

IMO, we need to optimize for screens all the way down to 320px width viewports based on https://www.paintcodeapp.com/news/ultimate-guide-to-iphone-resolutions.

serenamarie125 · 2018-05-29T18:38:40Z

FYI - PF guidance, is to only combine a verb with noun for Add/Create when sitting in a view/area that is clearly labeled (unless it's not associated with the main content of that view/area). In this case, it's clear that we are looking at Logs, so I think Download is works fine.

rhamilto · 2018-05-29T19:00:13Z

frontend/public/style/_common.scss

+    }
+  }
+
+  .co-m-pane__top-controls__group--center {


It appears the center option isn't being used? I recommend removing it and simplifying the layout if it's not in use.

ggreer · 2018-05-29T23:40:01Z

It really feels like this should be two pull requests: One to add the download button, and one to refactor layout/styling.

TheRealJon · 2018-05-30T13:40:20Z

@ggreer The style updates that I made were specifically related to adding a right-aligned download button to the top controls, so they go hand in hand. Since the co-m-pane__top-controls class isn't used in a lot of places, updating these styles didn't have many side effects.

TheRealJon · 2018-05-30T13:47:00Z

@rhamilto I changed the co-m-pane__top-controls class to just top-controls and adjusted the child classes as well. The controls also reflow at mobile widths so that the controls on the left stack. Posted an updated screen shot at 320px width.

rhamilto · 2018-05-30T14:16:26Z

Nice improvement, @TheRealJon!

I find it a bit odd that "Log streaming..." no longer sits next to the pause button. Is it possible to maintain the button and text on one line?

spadgett · 2018-05-30T15:17:53Z

frontend/public/style/_common.scss

+      justify-content: flex-end;
+    }
+
+    .top-controls__item {


I suggest we flatten this. We shouldn't need the nesting with BEM.

TheRealJon · 2018-05-30T16:30:42Z

Just updated the styles/structure of the top controls for the ResourceLog component. The message next to the TogglePlay button will not wrap @ mobile anymore. Also flattened the scss classes in _common.scss.

@rhamilto @spadgett @beanh66

beanh66 · 2018-05-30T17:20:31Z

Sweet, thanks @TheRealJon LGTM!

rhamilto · 2018-05-30T17:30:53Z

frontend/public/components/_toggle-play.scss

@@ -6,6 +6,10 @@
  width: 32px;
  padding: 0;

+  &--space-r {


Nit: I'm not a big fan of class names that describe what they do. I would rather see a class name that semantically describes the content rather than the presentation. Additionally, you could just put the margin right on ToggleButton without using the modifier as the other context that uses ToggleButton has an override that sets the margin to 0.

Yeah, I was trying to avoid adding a new class just for this context since this pattern might be used elsewhere. One option would be to make the status message an optional property of the TogglePlay component itself so that we can include styling in that component.

ggreer

Split this into two pull requests: One to add the download button, and one to refactor layout/styling.

spadgett · 2018-05-30T23:25:28Z

Split this into two pull requests: One to add the download button, and one to refactor layout/styling.

The PR is < 100 lines and has already been reviewed. Also you can't position the download button without changing the layout. Is this needed?

ggreer · 2018-05-31T01:37:02Z

Matt and I spent close to an hour trying to figure out the implications of this markup and styling. The combination of flexbox, media queries, nested elements, and padding tweaks... it all made us kinda give up trying to understand the exact differences regarding what users would see. I also wondered whether bootstrap or patternfly already had the necessary functionality. If not, perhaps we should choose a different UI pattern.

That said: The code behind the download functionality looks good, and this is a desirable feature. Had it been in its own PR with sub-optimal styling (along with a promise of a follow-up styling fix), I'm sure it would have been merged already. But when these two changes are coupled in the same PR, it tends to cause people to want to merge prematurely. (Since the sooner it's merged, the sooner the desirable feature ships.) I want to avoid that.

ggreer · 2018-05-31T01:46:58Z

jenkins rebuild

spadgett · 2018-05-31T15:08:34Z

@ggreer Do you have specific suggestions for how to make this simpler?

We were looking at it this morning, and flexbox seems reasonable. I'm not sure we can achieve the same layout with say Bootstrap grid. The intent is to make top-controls reusable for other pages so we only solve this once.

spadgett · 2018-05-31T15:12:39Z

pull-right doesn't work well because the vertical alignment is off, which flexbox fixes

rhamilto

LGTM pending squash.

ggreer · 2018-05-31T23:44:58Z

Here's a commit that gives the same behavior: fbb65d2

I pushed an image built with this commit (quay.io/coreos/tectonic-console:fbb65d276d4f24a9939a488505d9df7057121eda) and deployed it to teamui

spadgett · 2018-06-01T16:28:25Z

Thanks @ggreer. Appreciate that.

It's less code, but I have to point out that it doesn't reflow well, which is one of the problems we wanted to solve.

Also the "Log streaming..." text isn't vertically centered:

tlwu2013 · 2018-06-01T21:23:14Z

(Being late to the thread...)

I am wondering if it's better to have that "Download" btn being styled as a btn-link (with download icon)?
See OpenShift App Console, there is another feature for "expand to full screen". I worry if we add another button there will simply become visually too heavy.

See in OpenShift App console:

In Tectonic Console (and our previous GUI installer), there are also a few use cases for using btn-link. See:
0. Tectonic GUI Installer:

YAML sample sidebar:
YAML sidebar expand/collapse control:
Reveal data btn link Security details (introduced just recently):

ggreer · 2018-06-02T01:16:40Z

Oops, here's a revised commit with the log streaming text centered: bebb600 I pushed an image built with this commit (quay.io/coreos/tectonic-console:bebb60063127e4dca7cb28bf5ce1b0e75d470e04) and deployed it to teamui.

I realize the mobile styling isn't great, but log view on mobile has much bigger problems than the padding between buttons. For example, the viewable log area is barely larger than a postage stamp.

beanh66 · 2018-06-04T12:32:05Z

I am wondering if it's better to have that "Download" btn being styled as a btn-link (with download icon)?

@tlwu2013 We had this same debate while you were out and I believe ended up with a button to be consistent with Tectonic for now, but intended to revisit in the near future when we add other functionality like expand. Not sure we realized there was already some precedent for using links like this in Tectonic though. Having seen the screens you reference, I agree we could just go with a link from the start! @serenamarie125 @cshinn Thoughts?

spadgett · 2018-06-04T12:34:50Z

@TheRealJon Can you take @ggreer's latest commit for the styling? The only issue I see is that there's no margin between the items when they stack, but I think that shouldn't be too difficult to fix (either in this PR or a follow on).

@tlwu2013 I like using a link. If we change it here, should we change the download button on the YAML page, too?

I notice the case is different in "Reveal Values" compared to the others ("View samples"). I think we should fix that for consistency.

TheRealJon · 2018-06-04T15:21:38Z

@spadgett @ggreer Switched over to the simplified styles @ggreer suggested.

spadgett

/lgtm

Let's address any other changes in a follow on to get this merged.

spadgett · 2018-06-04T15:55:54Z

frontend/public/components/utils/resource-log.jsx

  return <div className="co-m-pane__top-controls">
    { status === 'loading' && <span className="co-icon-space-l"><LoadingInline /></span> }
    { ['streaming', 'paused'].includes(status) && <span className="log-stream-control"><TogglePlay active={status === 'streaming'} onClick={toggleStreaming}/></span>}
    <span className="log-container-selector__text">
      {logStatusMessages[status]}
    </span>
-    {dropdown && <span>{dropdown}</span>}
+    {dropdown && <span style={{flexGrow: 1}}>{dropdown}</span>}


We should avoid inline styles. (I know this was in @ggreer's commit.)

spadgett · 2018-06-04T15:56:03Z

frontend/public/components/utils/resource-log.jsx

-    {dropdown && <span>{dropdown}</span>}
+    {dropdown && <span style={{flexGrow: 1}}>{dropdown}</span>}
+    <button className="btn btn-default" onClick={onDownload}>
+      <i className="fa fa-download"></i>&nbsp;Download


aria-hidden="true"

stevekuznetsov · 2018-06-04T16:38:00Z

Merge blocking on the branch for this repo is causing our merge infrastructure to crash on this PR:

{"base-sha":"6805cee966d08b09a8ceee795016b060a6ec6351","branch":"master","component":"tide","controller":"sync","error":"1 review requesting changes and 2 approving reviews by reviewers with write access.","level":"error","msg":"Merge failed: PR is unmergable. How did it pass tests?!","org":"openshift","pr":77,"repo":"console","sha":"2fbd13414a8703c0590be130c8909c1620c14a6b","time":"2018-06-04T16:29:59Z"}

If you want merge automation we need to allow the bot to merge.

stevekuznetsov · 2018-06-04T16:39:43Z

/hold

(so the bot leaves it alone)

changes made

spadgett · 2018-06-04T17:38:19Z

/hold cancel

Add tests for Add Capacity and Storage System List Page

TheRealJon requested review from kans, spadgett and serenamarie125 May 29, 2018 17:03

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 29, 2018

rhamilto reviewed May 29, 2018

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 30, 2018

TheRealJon force-pushed the download-logs branch from 9150088 to ddc380c Compare May 30, 2018 13:57

spadgett reviewed May 30, 2018

View reviewed changes

rhamilto reviewed May 30, 2018

View reviewed changes

ggreer previously requested changes May 30, 2018

View reviewed changes

rhamilto approved these changes May 31, 2018

View reviewed changes

TheRealJon force-pushed the download-logs branch from 78e1421 to 6d81563 Compare June 4, 2018 15:17

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 4, 2018

TheRealJon force-pushed the download-logs branch from 6d81563 to c4b5121 Compare June 4, 2018 15:51

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2018

Implement download logs feature

2fbd134

TheRealJon force-pushed the download-logs branch from c4b5121 to 2fbd134 Compare June 4, 2018 15:53

spadgett approved these changes Jun 4, 2018

View reviewed changes

openshift-ci-robot assigned spadgett Jun 4, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2018

openshift-merge-robot merged commit 90f809b into openshift:master Jun 4, 2018

TheRealJon mentioned this pull request Jun 14, 2018

Fix styles for log view controls #130

Merged

TheRealJon deleted the download-logs branch March 12, 2019 13:17

christianvogt pushed a commit to christianvogt/console that referenced this pull request May 6, 2019

Fix delete modal from switching to admin perspective (openshift#77)

9ee62e7

christianvogt pushed a commit to christianvogt/console that referenced this pull request May 7, 2019

Fix delete modal from switching to admin perspective (openshift#77)

dcda0f4

TimothyAsirJeyasing pushed a commit to TimothyAsirJeyasing/console that referenced this pull request Aug 3, 2022

Merge pull request openshift#77 from bipuladh/install-test

d4c7209

Add tests for Add Capacity and Storage System List Page

Implement download button feature for logs #77

Implement download button feature for logs #77

Conversation

TheRealJon commented May 29, 2018 • edited

Description

Changes

Screenshots

robszumski commented May 29, 2018

TheRealJon commented May 29, 2018

spadgett commented May 29, 2018

TheRealJon commented May 29, 2018

rhamilto May 29, 2018

Choose a reason for hiding this comment

TheRealJon May 29, 2018 • edited

Choose a reason for hiding this comment

rhamilto commented May 29, 2018

serenamarie125 commented May 29, 2018

rhamilto May 29, 2018

Choose a reason for hiding this comment

ggreer commented May 29, 2018

TheRealJon commented May 30, 2018

TheRealJon commented May 30, 2018

rhamilto commented May 30, 2018

spadgett May 30, 2018

Choose a reason for hiding this comment

TheRealJon commented May 30, 2018 • edited

beanh66 commented May 30, 2018

rhamilto May 30, 2018 • edited

Choose a reason for hiding this comment

TheRealJon May 31, 2018

Choose a reason for hiding this comment

ggreer left a comment

Choose a reason for hiding this comment

spadgett commented May 30, 2018

ggreer commented May 31, 2018 • edited

ggreer commented May 31, 2018

spadgett commented May 31, 2018

spadgett commented May 31, 2018

rhamilto left a comment

Choose a reason for hiding this comment

ggreer commented May 31, 2018 • edited

spadgett commented Jun 1, 2018

tlwu2013 commented Jun 1, 2018

ggreer commented Jun 2, 2018

beanh66 commented Jun 4, 2018

spadgett commented Jun 4, 2018

TheRealJon commented Jun 4, 2018

spadgett left a comment

Choose a reason for hiding this comment

spadgett Jun 4, 2018

Choose a reason for hiding this comment

spadgett Jun 4, 2018

Choose a reason for hiding this comment

stevekuznetsov commented Jun 4, 2018

stevekuznetsov commented Jun 4, 2018

spadgett commented Jun 4, 2018

TheRealJon commented May 29, 2018 •

edited

TheRealJon May 29, 2018 •

edited

TheRealJon commented May 30, 2018 •

edited

rhamilto May 30, 2018 •

edited

ggreer commented May 31, 2018 •

edited

ggreer commented May 31, 2018 •

edited