[top nav] Enhancements to make nav item state more dynamic#7630
[top nav] Enhancements to make nav item state more dynamic#7630ycombinator merged 12 commits intoelastic:masterfrom
Conversation
|
Build (on old/current Jenkins) is passing after the latest commit (ed0c48a): http://build-eu-00.elastic.co/job/kibana_core_pr/4488/ |
| border-radius: 0; | ||
| } | ||
|
|
||
| button.is-kbn-top-nav-button-disabled { |
There was a problem hiding this comment.
Is it possible to simplify this to just the class .is-kbn-top-nav-button-disabled?
There was a problem hiding this comment.
I tried that first but it didn't apply the style. I had to add button to make the selector more specific.
|
1 question, then 👍 |
| run: (item) => !item.disableButton() && this.toggle(item.key) | ||
| }); | ||
|
|
||
| defaultedOpt.hideButton = isFunction(opt.hideButton) ? opt.hideButton : () => (opt.hideButton || false); |
There was a problem hiding this comment.
You can use _.result for all of these.
For example: defaultedOpt.hideButton = result(opt, 'hideButton', false);
There was a problem hiding this comment.
Ah, thanks for finding that function. I looked through the lodash docs but result didn't sound like the thing I was looking for :)
|
This is a funny side effect of the function handling and the digest loop 🍔
Not a problem, just thought it was fun and wanted to share. Aside from that change to using |
|
jenkins, test it |
e5e2ca2 to
46c37c0
Compare
46c37c0 to
cf99029
Compare
| // apply the defaults to individual options | ||
| _applyOptDefault(opt = {}) { | ||
| return defaults({}, opt, { | ||
| const defaultedOpt = defaults({}, opt, { |
There was a problem hiding this comment.
I think this can be rewritten without defaults:
const defaultedOpt = Object.assign({
label: capitalize(opt.key),
hasFunction: !!opt.run,
description: opt.run ? opt.key : `Toggle ${opt.key} view`,
run: (item) => !item.disableButton && this.toggle(item.key)
}, opt);|
Looks great, just had 1 suggestion and 1 question. |
|
LGTM! |
|
Changing to
Nice! LGTM!!! |
…ents [top nav] Enhancements to make nav item state more dynamic Former-commit-id: c2e99e3
Currently the state of a top nav item (specifically whether is should be hidden or not, via the
hideButtonproperty) can be set only once, when that item is registered. This PR introduces the following enhancements for top nav items:hideButtonproperty to be a function that returns a boolean value, instead of a scalar boolean value. If a scalar boolean value is passed in, it will be converted to a function that returns that boolean value; this preserves backwards-compatibility as well.disableButtonproperty, similar in syntax to thehideButtonproperty. When this property evaluates totrue, the top nav button is disabled (grayed out and unclickable); else the top nav button appears and functions normally.tooltipproperty, similar in syntax to thehideButtonproperty. This allows a tooltip to be shown on the top nav item.