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

Finalize drilldowns 🎉 #59973

Closed
41 of 45 tasks
streamich opened this issue Mar 12, 2020 · 9 comments · Fixed by #59632
Closed
41 of 45 tasks

Finalize drilldowns 🎉 #59973

streamich opened this issue Mar 12, 2020 · 9 comments · Fixed by #59632
Assignees
Labels
Feature:Drilldowns Embeddable panel Drilldowns

Comments

@streamich
Copy link
Contributor

streamich commented Mar 12, 2020

  • Make number in "Manage drilldowns" pink.
  • Figure out what to do with context menu separator.
  • Don't make context in Presentable methods optional.
  • Do not use uiToReactComponent in JSX.
  • Do not show action factories of already created drilldowns.
  • Disable "Create new" button in management screen when no more action factories.
  • Do not use factory icon in action instance, but instance should have its own icon.
  • Clean-up ActionInternal class.
  • Do not show "0" in edit mode in panel top right corner on no drilldowns.
  • When clicking on pie chart filtering does not work without drilldown.
  • For panel with drilldown, edit mode is wrong.
  • When adding drilldown to panel, time range badge is added, too.
  • Order correctly action factories.
  • top Make drilldown counter/dot in panel's right-top corner reactive.
  • Why is there delay when clicking "Delete" button? @Dosant: What delay? 🤔
  • Figure out drilldown factory context.
  • Disable drilldowns for embeddables that don't have .dynamicActions.
  • Make use of supportedTriggers() from embeddable.
  • After panel context menu random order has been fixed, check what is the intended ordering of the context menu items. Drill down action panel order #60138
  • Close drilldown flyout when navigating to another app. (Issue with all panels. Fixed by
  • Looks like browser history is ignoring drilldowns changes.
  • Pass in context to configuration component.
  • Clean up UiActionsService.
  • Fix usage of attachAction.

UI

Questions to Design:

@andreadelrio will look into fixing these in EUI

  • Align left/right content in <HelloBar>.
  • Make font size smaller.
  • Make icon grey color.

image

@Dosant: For used example from EUI trying to avoid any additional custom css on top. @andreadelrio, should we fix alightment issue in EUI? Not sure about font size and grey icon as used https://github.com/elastic/kibana/blob/drilldowns/x-pack/plugins/drilldowns/public/components/drilldown_hello_bar/drilldown_hello_bar.tsx

  • Align action factory picker. (Should be fixed now.)

QA

  • Add unit tests
    • DynamicActionManager
    • UI Actions service
      • Action factory
      • Action registration/unregistration
    • Open flyout actions
      • FlyoutCreateDrilldownAction
      • FlyoutEditDrilldownAction
  • Functional test suite - @Dosant [Drilldowns] Basic functional test #60244
  • Test
    • In Internet Explorer
    • In various browsers
    • In accessibility mode
    • In incognito mode (make sure localStorage does not throw)
    • In dark mode

Can be done in 7.8

Moved to #60254


Parent issue: #55349

@streamich streamich mentioned this issue Mar 12, 2020
7 tasks
@streamich streamich added Feature:Drilldowns Embeddable panel Drilldowns Team:AppArch labels Mar 12, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@streamich streamich changed the title Drilldown finalization laundry list Finalize drilldowns 🎉 Mar 12, 2020
@andreadelrio
Copy link
Contributor

@Dosant @streamich I'll submit a PR for this on Monday, need to sync up with DeFazio first as I noticed he changed some things on the mockups recently.

@andreadelrio
Copy link
Contributor

  • Fix separator line

image

Parent issue: #55349

For this, can we pass the euiHorizontalRule alone instead of inside of the euiContextMenuItem button?

image

This was referenced Mar 16, 2020
@Dosant
Copy link
Contributor

Dosant commented Mar 16, 2020

@andreadelrio, for this component we are using https://elastic.github.io/eui/#/navigation/context-menu directly and passing a panel with a set of preconfigured panel items. I don't see a way to squeeze in euiHorizontalRule without a button there using just EuiContextMenuItemProps. Do you?

We should at least make the button disabled, which makes this solution a bit better then it now and solves issue with redundant focus state / pointer cursor and etc.

We could also consider removing this hacky separator implementation for now and add support for that in eui first or just later jump straight to Option 2 implementation with multiple panels proposed here: #60138

@andreadelrio
Copy link
Contributor

@andreadelrio, for this component we are using https://elastic.github.io/eui/#/navigation/context-menu directly and passing a panel with a set of preconfigured panel items. I don't see a way to squeeze in euiHorizontalRule without a button there using just EuiContextMenuItemProps. Do you?

We should at least make the button disabled, which makes this solution a bit better then it now and solves issue with redundant focus state / pointer cursor and etc.

We could also consider removing this hacky separator implementation for now and add support for that in eui first or just later jump straight to Option 2 implementation with multiple panels proposed here: #60138

@Dosant I'd say let's hold off on adding the separator for now and I'll look into adding support to in on Eui.

@streamich
Copy link
Contributor Author

streamich commented Mar 17, 2020

@Dosant @andreadelrio regarding that left side alignment:

image

Do I understand correctly that we will move the icon to the right in EUI itself?

image

I guess the left chevron we need to still move in Kibana?

image

@streamich
Copy link
Contributor Author

@Dosant @andreadelrio actually I've fixed, I guess we don't need to do anything in EUI.

image

@Dosant
Copy link
Contributor

Dosant commented Mar 17, 2020

@streamich, I think the point just was to not add extra redundant custom css, but fix it properly in https://elastic.github.io/eui/#/layout/flyout including Flyout with banner example

@streamich
Copy link
Contributor Author

Once we have that fix we can remove the CSS from the hello message icon. But for flyout header chevron icon we still need that CSS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants