Make sure no one can show the chrome if the default setting is hidden. #13250
Conversation
stacey-gammon
added
Feature:Dashboard
There was a problem hiding this comment.
Looks like there is a broken test. Not sure if this change cased that.
"dashboard app dashboard tab add new visualization link adds a new visualization"
03:23:29.917 │ tryForTime timeout: Error: tryForTime timeout: ElementNotVisible: An element command could not be completed because the element is not visible on the page
src/ui/public/chrome/api/controls.js
Outdated
| @@ -19,6 +19,10 @@ export default function (chrome, internals) { | |||
| * @return {chrome} | |||
| */ | |||
| chrome.setVisible = function (display) { | |||
| if (!def) { | |||
There was a problem hiding this comment.
Not sure if this is worth changing in this PR, but the legacy of controls.js and the def variable are confusing. To me, a default is a value that can be changed. Since the method setVisible is provided, I would assume that default could be changed. And - to an external consumer, how do setVisible and setVisibleDefault relate to one-another.
Maybe def could be changed to noChromeMode and setVisibleDefault could be renamed setNoChromeMode. That way, the behavior of the variable would be better reflected in the name.
elastic#13250) * Make sure no one can show the chrome if the default setting is hidden. add tests Fixes elastic#13040 * Improve variable and function names and fix tests
elastic#13250) * Make sure no one can show the chrome if the default setting is hidden. add tests Fixes elastic#13040 * Improve variable and function names and fix tests
and add tests to ensure this doesn't happen again.
Fixes #13040