EUIify dashboard options top nav #21510
Conversation
nreese
commented
Jul 31, 2018
nreese
commented
nreese
added
Feature:Dashboard
💚 Build Succeeded |
| hidePanelTitles, | ||
| onHidePanelTitlesChange, | ||
| }) { | ||
| if (isOpen) { |
There was a problem hiding this comment.
Maybe consider renaming showOptionsPopover to toggleOptionsPopover? If I'm reading this line correctly. It sometimes shows, sometimes hides, depending on the value of isOpen.
Also, you might just make this function body something like this, for consistency:
export function toggleOptionsPopover(props) {
if (isOpen) {
closeOptionsPopover();
} else {
openOptionsPopover(props);
}
}shrugs Not important.
| EuiSwitch, | ||
| } from '@elastic/eui'; | ||
|
|
||
| export class OptionsMenu extends Component { |
There was a problem hiding this comment.
Any chance you can change this file to tsx? Since it has no imports, I think it should be straightforward (assuming eui has the imported types defined... if it doesn't, then forget it).
You can use https://github.com/elastic/kibana/pull/21740/files#diff-15359b57331d6e7ede80dbc9fc90396c as a reference - I think it's a very similar conversion.
| .euiPopover__anchor { | ||
| height: 100%; | ||
| } | ||
| } |
There was a problem hiding this comment.
cc @cchaos - here are some LESS changes that will probably go in before your LESS to SASS conversion. Any objections? Hopefully not to difficult to merge?
There was a problem hiding this comment.
Yeah that will be fine. I'll be able to handle the rebase.
| isOpen = false; | ||
| }; | ||
|
|
||
| export function showOptionsPopover({ |
There was a problem hiding this comment.
if you convert the other file to typescript, this one should be relatively easy to convert too, since that other file is the only import.
There was a problem hiding this comment.
Ideally I'd love to see the new files that have no dependencies converted to typescript, but other than that, LGTM. Pulled the code and tested to see if URL modifications were reflected (they were - though you had to close and open the panel to see the modifications - I think that's fine).
Updating some of the options is slow, but compared with master and it seems to be the same, if the dashboard has a lot of heavy visualizations on it.
tl;dr; LGTM!
|
I think I will leave typescripting for another PR to limit the scope of this PR. Typescripting core APIs is really helpful. However, I feel that typescripting everything has limited benefits and even makes things more confusing. Edge components like these should be simple and as close to pure javascript as possible. React already provides type checking via PropTypes that is simple and has a wider breadth of audience. That seems like a good enough solution for the moment. |
💚 Build Succeeded |
* trying new EuiWrappingPopover * allow button press to close popover * fix button styling * start options panel with darkTheme option * add other options * remove old options html * get dashboar state functional test working
