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

[Metrics-UI] Fix toolbar popover for metrics table row #56796

Merged
merged 8 commits into from
Feb 7, 2020

Conversation

phillipb
Copy link
Contributor

@phillipb phillipb commented Feb 4, 2020

Summary

When the popover opens, set overflow to hidden. Update the code to use hooks.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@phillipb phillipb added release_note:fix Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 labels Feb 4, 2020
@phillipb phillipb requested a review from a team as a code owner February 4, 2020 20:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@phillipb
Copy link
Contributor Author

phillipb commented Feb 5, 2020

@elasticmachine merge upstream

@afgomez afgomez self-requested a review February 6, 2020 09:49
Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

I wanna make sure I'm reviewing the right thing: this PR tackles the popover when the user clicks on an elemenent in the metrics table, right?

Screenshot 2020-02-06 at 10 55 25

<EuiButtonEmpty onClick={handleClick}>{value}</EuiButtonEmpty>
export const TableView = (props: Props) => {
const { nodes, options, formatter, currentTime, nodeType } = props;
const [isPopoverOpen, setIsPopoverOpen] = useState<string[]>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I see this was the previous name, but I'd rename this to openPopovers or similar. My first reaction with the current name is that:

  • There's only one popover
  • The variable is a boolean


const closePopoverFor = useCallback(
(id: string) => () => {
setIsPopoverOpen([...isPopoverOpen, id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can drop this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@phillipb
Copy link
Contributor Author

phillipb commented Feb 6, 2020

I wanna make sure I'm reviewing the right thing: this PR tackles the popover when the user clicks on an elemenent in the metrics table, right?

@afgomez Yeah, that's the right one

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@elastic elastic deleted a comment from kibanamachine Feb 6, 2020
@afgomez afgomez changed the title [Metrics-UI] Fix toolbar hover for metrics table row [Metrics-UI] Fix toolbar popover for metrics table row Feb 7, 2020
Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

Ship it 🎉!

@phillipb phillipb merged commit dd8ccc4 into elastic:master Feb 7, 2020
phillipb added a commit to phillipb/kibana that referenced this pull request Feb 7, 2020
* Disable scroll when popover is open. Switch to hooks

* Remove unused prop

* UseEffect to update styles

* Remove ununsed import

* Rename variables for clarity

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@phillipb
Copy link
Contributor Author

phillipb commented Feb 7, 2020

@afgomez thanks for the review!

phillipb added a commit that referenced this pull request Feb 7, 2020
* Disable scroll when popover is open. Switch to hooks

* Remove unused prop

* UseEffect to update styles

* Remove ununsed import

* Rename variables for clarity

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 9, 2020
…t-state

* upstream/master: (96 commits)
  top nav ts arg support (elastic#56984)
  [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130)
  Add docs for alerting and action settings (elastic#57035)
  Add Test to Verify Endpoint App Landing Page (elastic#57129)
  Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126)
  chore(NA): removes use of parallel option in the terser minimizer (elastic#57077)
  [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972)
  Specifying valid licenses for the Graph feature (elastic#55911)
  [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948)
  [ML] DF Analytics creation: update schema definition for create route (elastic#56979)
  Remove Kibana a11y guide in favor of EUI (elastic#57021)
  [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329)
  [docs] Fix spaces api example json (elastic#50411)
  Add new config for filebeat index name (elastic#56920)
  [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796)
  Saved Objects testing (elastic#56965)
  Disabled categorization stats validation (elastic#57087)
  [Rollups] Server NP migration (elastic#55606)
  [Metrics UI] Limit group by selector to only 2 fields (elastic#56800)
  fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 9, 2020
* master: (96 commits)
  top nav ts arg support (elastic#56984)
  [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130)
  Add docs for alerting and action settings (elastic#57035)
  Add Test to Verify Endpoint App Landing Page (elastic#57129)
  Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126)
  chore(NA): removes use of parallel option in the terser minimizer (elastic#57077)
  [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972)
  Specifying valid licenses for the Graph feature (elastic#55911)
  [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948)
  [ML] DF Analytics creation: update schema definition for create route (elastic#56979)
  Remove Kibana a11y guide in favor of EUI (elastic#57021)
  [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329)
  [docs] Fix spaces api example json (elastic#50411)
  Add new config for filebeat index name (elastic#56920)
  [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796)
  Saved Objects testing (elastic#56965)
  Disabled categorization stats validation (elastic#57087)
  [Rollups] Server NP migration (elastic#55606)
  [Metrics UI] Limit group by selector to only 2 fields (elastic#56800)
  fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants