Skip to content

Conversation

@YulNaumenko
Copy link
Contributor

Resolve #60765

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// labels Mar 24, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner March 24, 2020 16:28
@YulNaumenko YulNaumenko self-assigned this Mar 24, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikecote mikecote self-requested a review March 24, 2020 16:50
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cards endup having different heights, I wonder if @andreadelrio found a workaround for this as she also noticed the issue when no betaBadgeLabel was defined.

Screen Shot 2020-03-24 at 12 51 50 PM

@mikecote
Copy link
Contributor

mikecote commented Mar 24, 2020

TIL, the badge was there to solve the height differences issue.

I'm looking for alternatives:

  • What if we rename the badge to "Disabled"?
  • What if we provide the EuiCard content ourselves and wrap that within a tooltip (may be less UX as user would have to hover over text to see tooltip)
  • Set a fixed height to the cards?

cc @alexfrancoeur @peterschretlen

…pgrade-license-budge

# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form/action_type_menu.test.tsx
@alexfrancoeur
Copy link

Hmm disabled sounds like you can enable it. If we need the badge, what about "Not available"? Would be interesting in @gchaps and @arisonl thoughts.

@alexfrancoeur
Copy link

alexfrancoeur commented Mar 24, 2020

If have the banner on top with the appropriate CTA (#60698), then it should be self explanatory as to why they are not available / disabled / [whatever text we choose]

@YulNaumenko YulNaumenko requested a review from a team as a code owner March 24, 2020 17:58
@mdefazio
Copy link
Contributor

PR to just add some css that fixes the height issue
YulNaumenko#2

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick update on this!

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@YulNaumenko YulNaumenko merged commit f9088eb into elastic:master Mar 24, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Mar 24, 2020
…or flyout (elastic#61111)

* Remove "Upgrade" badge on disabled action types in the create connector flyout

* Fix height of disabled cards

Co-authored-by: defazio <[email protected]>
YulNaumenko added a commit that referenced this pull request Mar 24, 2020
…or flyout (#61111) (#61147)

* Remove "Upgrade" badge on disabled action types in the create connector flyout

* Fix height of disabled cards

Co-authored-by: defazio <[email protected]>

Co-authored-by: defazio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.7.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove "Upgrade" badge on disabled action types in the create connector flyout

7 participants