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

[EuiBreadcrumb] Allow popover content to close the breadcrumb popover #7555

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Mar 6, 2024

Summary

@sebelga pinged me with the request for serverless nav work to be given the ability to optionally control EUI's breadcrumb popover state. His use case is for consumer popover content that does something/takes the user somewhere and then closes the popover, e.g.

The approach I decided to take here was to allow popoverContent to be a render function that passes the closePopover callback to consumers. Example usage:

<EuiBreadcrumbs
  breadcrumbs={[
    {
      text: 'My deployment',
      popoverContent: (closePopover) => (
        <EuiButtonEmpty
          onClick={() => {
            goSomewhere();
            closePopover();
          }}
        >
          Go to some deployment
        </EuiButtonEmpty>
      ),
    },
    { text: 'Another breadcrumb' },
  ]}
/>

I'm open to feedback as to whether we think this is the best approach in terms of giving consumers more flexibility vs handling internals for consumers so they don't have to.

QA

General checklist

  • Browser QA
    - [ ] Checked in both light and dark modes
    - [ ] Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
  • Designer checklist - N/A

@kibanamachine
Copy link

Preview staging links for this PR:

@cee-chen cee-chen marked this pull request as ready for review March 6, 2024 17:05
@cee-chen cee-chen requested a review from a team as a code owner March 6, 2024 17:05
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen self-assigned this Mar 6, 2024
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! I tested with keyboard and mouse clicks in four evergreen browsers on MacOS.

@cee-chen
Copy link
Member Author

cee-chen commented Mar 6, 2024

Going to go ahead and merge this as Sebastian has approved the render function architecture as meeting his use case - if anyone else on the team feels strongly that complete consumer control would be better, please leave a comment before next week's release and I can revisit/revert as necessary.

Edit: Actually I'm going to hold on merging until tomorrow to give the EU folks time to review/leave comments, but will merge by my EOD Thursday!

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

To me the updates make sense. 👍 It allows for more external control without opening all flood gates and being too invasive. Imho, if we find in the future that more control would be needed it could still be assessed then.

@cee-chen
Copy link
Member Author

cee-chen commented Mar 7, 2024

Thanks a ton Lene, totally agreed!

@cee-chen cee-chen merged commit 415971d into elastic:main Mar 7, 2024
8 checks passed
@cee-chen cee-chen deleted the breadcrumbs/popover-render-function branch March 7, 2024 16:38
cee-chen added a commit to elastic/kibana that referenced this pull request Mar 12, 2024
`v93.2.0`⏩`v93.3.0`

---

## [`v93.3.0`](https://github.com/elastic/eui/releases/v93.3.0)

- Added new `EuiDataGrid` new prop: `cellContext`, an optional object of
additional props passed to the cell render function.
([#7374](elastic/eui#7374))
- `EuiBreadcrumbs`'s `popoverContent` API now accepts a render function
that will be passed a `closePopover` callback, allowing consumers to
close the breadcrumb popover from their popover content
([#7555](elastic/eui#7555))

**Bug fixes**

- Fixed missing animation on native `EuiProgress` bar update
([#7538](elastic/eui#7538))
- Fixed an `EuiDataGrid` bug with `gridStyle.rowClasses`, where custom
consumer classes that began with `euiDataGridRow` would not be correctly
removed/reapplied ([#7549](elastic/eui#7549))
- Fixed a visual `EuiDataGrid` bug where `EuiCheckbox`es within control
columns were not vertically centered within single height rows
([#7549](elastic/eui#7549))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants