Beats/ui tweaks #23655
Beats/ui tweaks #23655
Conversation
…kibana into beats/ui-tweaks # Conflicts: # x-pack/plugins/beats_management/public/components/tag/tag_assignment.tsx # x-pack/plugins/beats_management/public/pages/main/beats.tsx
…kibana into beats/ui-tweaks
💔 Build Failed |
|
Si, this is the thing I was saying would go into a follow up PR after you merge your table refactor (along with the table on beats details doing the same) |
| name: 'Beats Activity', | ||
| disabled: false, | ||
| }, | ||
| // { |
There was a problem hiding this comment.
Do we need to keep this object commented for now or can it be removed?
| <div> | ||
| <EuiButtonEmpty | ||
| onClick={() => { | ||
| // random, but spacific number ensures new tab does not overwrite another _newtab in chrome |
There was a problem hiding this comment.
"random, but specific"
There was a problem hiding this comment.
hahah mash the keyboard. "random" in that its not 3000 or something another dev of another app might have used. Specific in that its hard coded and not randomly generated. Ion this way if a user clicks this link 20 times its not going to open 20 tabs. Also it will not overwrite another tab or the URL changed by another app opening something in a new tab if someone leaves the tab open
There was a problem hiding this comment.
Ah sorry - I should have been clearer, I just meant there's a typo in the comment, "specific" was misspelled 👼
| name: 'Beat Activity', | ||
| disabled: false, | ||
| }, | ||
| // { |
There was a problem hiding this comment.
Do we need to keep this object commented out for now, or can it be removed?
There was a problem hiding this comment.
It will be the very first thing we do in phase 2... up to you. I tend to over comment things out
| state.currentRequest && | ||
| state.currentRequest.expression !== expression && | ||
| state.currentRequest.cursorPosition !== cursorPosition | ||
| ? state // ignore this result, since a newer request is in flight |
There was a problem hiding this comment.
Comments like this in these longer one-liners are helpful. It may be useful to include more, like in withSuggestionAtIndexSelected in the autocomplete component.
| render={() => <Redirect from="/" exact={true} to="/overview/beats" />} | ||
| <div> | ||
| <Header | ||
| breadcrumbs={[ |
There was a problem hiding this comment.
Love that we've added breadcrumbs, it will make the navigation UX much better.
Are you planning to add more functionality to this at a later time? Right now when I click into a beat's details view, it doesn't add a crumb for my selected beat, and the only functional link is the Management one that takes me out of CM altogether.
There was a problem hiding this comment.
Ah, yea... ill add that as a TODO before GA
| breadcrumbs?: EuiBreadcrumbDefinition[]; | ||
| } | ||
|
|
||
| export class Header extends React.PureComponent<HeaderProps> { |
There was a problem hiding this comment.
So happy to have this UX addition 🥇
| @@ -43,6 +60,14 @@ export function ControlBar(props: ControlBarProps) { | |||
| /> | |||
| ) : ( | |||
| <PrimaryOptions | |||
| onKueryBarSubmit={onKueryBarSubmit} | |||
There was a problem hiding this comment.
It seems like a lot of these props are being referenced and then passed through so they can be used in other components.
In the re-arch I'm working on, I've compressed a lot of the assignment options fields into an optional prop that is defined in the table module and passed along. Do you think it would be useful to employ a similar approach here, since it seems like a lot of these props are unused by the intermediary components?
There was a problem hiding this comment.
@justinkambic Great question. So the props here are borrowed from whats being used in InfraOps. Perhaps a destruct/spread would be better? That way its passed in how the shared HOC passes in, but we only need to pass through a single prop?
There was a problem hiding this comment.
👍 that would be good. Can be done in a follow-up, I don't know that we have to halt the changes based on that here, given the timeline for what we want to get merged this week
| items: any[]; | ||
| renderAssignmentOptions?: (item: any, key: string) => any; | ||
| showAssignmentOptions: boolean; | ||
| type: TableType; | ||
|
|
||
| isLoadingSuggestions: boolean; |
There was a problem hiding this comment.
Similar comment as in the controls file about passing these props through. Could we define an interface for these and pass them through as an object, or include the interface definition when assigning the props type to the class, to keep the table's props definitions cleaner, and only related to what it needs to function?
There was a problem hiding this comment.
similar answer 😊
Many UI tweaks + autocomplete search on table