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

Chore: tweak header controls #35848

Merged
merged 3 commits into from
May 1, 2019
Merged

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Apr 30, 2019

Summary

This PR just changes the controls in the workpad header slightly, such that the refresh control is always accessible. The settings for that control were moved to a new settings component (and icon), but the content of the popover is mostly unchanged.

Pre-req for Kiosk mode, since I plan to add those settings next to the refresh controls in the popover.

This is also part of some broader changes we'd like to make to the controls.

Before

Apr-30-2019 16-22-03-optimized

After

Apr-30-2019 16-24-36-optimized

@w33ble w33ble added review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v7.2.0 labels Apr 30, 2019
@w33ble w33ble requested a review from ryankeairns April 30, 2019 23:30
@w33ble w33ble requested review from a team as code owners April 30, 2019 23:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

<EuiButton size="s" fill iconType="refresh" onClick={doRefresh} isDisabled={inFlight}>
Refresh
</EuiButton>
<RefreshControl />
Copy link
Contributor Author

@w33ble w33ble Apr 30, 2019

Choose a reason for hiding this comment

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

The refresh button in the popover is an intentional duplication, since we plan to swap out that control at the top based on the refresh state; see #35849.

}}
/>
</div>
<EuiFlexGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using flex here so that I can put more setting next to this in a new <EuiFlexItem>

@@ -0,0 +1,3 @@
.canvasControlSettings__popover {
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 is just a replacement for refresh_control.scss, the style on the base class that used to be there wasn't used, nothing had that class.

@w33ble w33ble force-pushed the chore/tweak-header-controls branch 3 times, most recently from a9ef6a4 to e37971a Compare May 1, 2019 00:06
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks good. I like bringing all the individual components under workpad_header...until we've got a reason to move them back out.

👍

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of minor tweaks. Let's remove the <dd> and/or change the <dt> to an <li>. The current formatting looks like a title that applies to the settings below, but the bottom half pertains to 'automated' refresh.

I am also suggesting we change the text up top to be more about refreshing the element data. The current wording struck me as reloading the browser since it's such a common phrase.

With those changes, we end up with something looking like this:
Screenshot 2019-05-01 09 35 00

@cchaos
Copy link
Contributor

cchaos commented May 1, 2019

Just as an FYI, there has been some discussion around the list display of the time picker using EuiFlexGrid (#35684) because it causes the order to zig-zag vs column first.

You may want to consider also tweaking this design so that it's left column first like:

@w33ble
Copy link
Contributor Author

w33ble commented May 1, 2019

@ryankeairns This is kind of out of scope for this PR, and I'm really not sure what you're recommending I change, other than the wording at the top. I liked the idea of re-ordering the presets, so I did that. Here's where I've landed with it; the default state:

Screenshot 2019-05-01 10 48 58

And then when you set auto-refresh:

Screenshot 2019-05-01 10 49 07

@ryankeairns
Copy link
Contributor

@w33ble that's what I get for lazily just changing things in inspector and not actually poking at the code. I'll give it a quick go and see if I can't change it myself... be right back.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

w33ble added 3 commits May 1, 2019 11:59
the components are only used there, and aren't really intended to be reused anywhere else
separate and use refresh control, move popover to settings control
@w33ble w33ble force-pushed the chore/tweak-header-controls branch from 350bf17 to d4e8cab Compare May 1, 2019 18:59
@ryankeairns
Copy link
Contributor

ryankeairns commented May 1, 2019

@w33ble the latest wording changes you made addressed my initial confusion up in the title area. I'll save the visual edits for a separate PR. Thanks!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@w33ble w33ble merged commit f340dab into elastic:master May 1, 2019
w33ble added a commit to w33ble/kibana that referenced this pull request May 2, 2019
* chore: move control components into workpad_header

the components are only used there, and aren't really intended to be reused anywhere else

* chore: change header controls

separate and use refresh control, move popover to settings control

* chore: tweak refresh setting appearance
w33ble added a commit that referenced this pull request May 2, 2019
* chore: move control components into workpad_header

the components are only used there, and aren't really intended to be reused anywhere else

* chore: change header controls

separate and use refresh control, move popover to settings control

* chore: tweak refresh setting appearance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants