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

[Chrome] Fix extension truncation #83000

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Nov 9, 2020

Summary

In #82015 we introduced new extension point for chrome that allows externally append an element to the last breadcrumb.

That is needed for future send to background UI: #81004
Mocks https://whimsical.com/LMKSnR7vmPrmAypPXYuA8T

I noticed a bug, that extension is not visible in case last breadcrumb is truncated (for example on a mobile device inside a dashboard app with a long dashboard title).

This pr attempt to fix it:

Screenshot 2020-11-09 at 19 16 06

Checklist

Delete any items that are not applicable to this PR.

  • This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)

For maintainers

@Dosant Dosant added bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes labels Nov 9, 2020
@Dosant Dosant marked this pull request as ready for review November 10, 2020 10:49
@Dosant Dosant requested review from a team as code owners November 10, 2020 10:49
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant requested a review from pgayvallet November 10, 2020 10:49
@Dosant Dosant added v7.11.0 and removed 7.11.0 labels Nov 10, 2020
src/core/public/chrome/ui/header/header_breadcrumbs.tsx Outdated Show resolved Hide resolved
Comment on lines 68 to 59
<EuiFlexGroup responsive={false} gutterSize={'none'} className="eui-textTruncate">
<EuiFlexItem grow={false} className="eui-textTruncate">
{Breadcrumbs}
</EuiFlexItem>
<EuiFlexItem grow={true} className="chrHeaderNavBreadcrumbs__appendExtensionContainer">
<HeaderExtension extension={breadcrumbsAppendExtension.content} />
Copy link
Contributor

Choose a reason for hiding this comment

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

In #82015 (review) we discussed about a future improvement to allow plugins to mutate the breadcrumb. I think it would be easier to migrate to it later if you were altering the last breadcrumb element instead of creating a flexGroup around it.

Would somethink like

lastCrumb.text = (
    <EuiFlexGroup responsive={false} gutterSize={'none'} className="eui-textTruncate">
      <EuiFlexItem grow={false} className="eui-textTruncate">
        {lastCrumb.text}
      </EuiFlexItem>
      <EuiFlexItem grow={true} className="chrHeaderNavBreadcrumbs__appendExtensionContainer">
        <HeaderExtension extension={breadcrumbsAppendExtension.content} />
       </EuiFlexItem>
    </EuiFlexGroup>
);

solves the truncation on small width, or are you forced to have the extension outside of the breadcrumb element?

Copy link
Contributor Author

@Dosant Dosant Nov 10, 2020

Choose a reason for hiding this comment

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

That've almost worked:

Screenshot 2020-11-10 at 12 26 53

Just no ... after the title :(

Copy link
Contributor

Choose a reason for hiding this comment

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

text-overflow is always a pain to use properly. Anyway, as @kibana-design has been requested for review, maybe they could take a look. Else, keeping it as it is would be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason the text-overflow: ellipsis; isn't being honored here is because the EuiFlexItem components have a style of display: flex;. Changing the offending EuiFlexItem to display: block; should remedy it (if this is a path you would prefer to take).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaelMarcialis, thanks for the tip! it helped and I made it this way.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Hello. I've left two small comments for your review.

src/core/public/chrome/ui/header/header_breadcrumbs.tsx Outdated Show resolved Hide resolved
src/core/public/chrome/ui/header/header_breadcrumbs.tsx Outdated Show resolved Hide resolved
@Dosant Dosant force-pushed the dev/chrome/appendBreadcrumbExtensionTruncationFix branch from fbfa072 to 29e6c97 Compare November 11, 2020 12:31
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 547.3KB 547.4KB +151.0B

History

  • 💚 Build #87043 succeeded fbfa07215d9e5e3b57f338090669d04f9d0edb35
  • 💚 Build #86896 succeeded 70d67dbb3bab69803f911ea6898bc4d0a057c6c9
  • 💔 Build #86888 failed 5876f16c50fdfbdf12a499c851405019b7e8983d

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

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -51,15 +51,14 @@ export function HeaderBreadcrumbs({ appTitle$, breadcrumbs$, breadcrumbsAppendEx
),
}));

if (breadcrumbsAppendExtension) {
if (breadcrumbsAppendExtension && crumbs[crumbs.length - 1]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: isn't bool(crumbs[crumbs.length - 1]) just bool(crumbs.length) but harder to read 😅 ?

@MichaelMarcialis
Copy link
Contributor

This appears to work as advertised. Before giving my stamp of approval, CCing @cchaos for her opinions regarding:

  1. Is this usage of EuiFlexGroup without subsequent EuiFlexItem acceptable? I don't see why it wouldn't be, but thought I'd ask, as this is the first time I'm seeing it used this way.
  2. Would this pushing of the modified last breadcrumb markup into the breadcrumb component's markup be frowned upon?

@cchaos
Copy link
Contributor

cchaos commented Nov 11, 2020

@MichaelMarcialis To answer your specific questions:

  1. It's ok to not use EuiFlexItem so long as the layout your creating is intended with EuiFlexGroup
  2. It is less that appending a custom breadcrumb is frowned upon than that this is causing some render issues.

For instance the text alignment is not quite right

Screen Shot 2020-11-11 at 11 31 01 AM

And in the new Amsterdam theme, the styles are especially off.

Screen Shot 2020-11-11 at 11 32 42 AM

We can merge this for now, but most likely, EUI will have to provide something on the component side to support this directly.

@Dosant Dosant merged commit b8576ed into elastic:master Nov 12, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request Nov 12, 2020
Dosant added a commit that referenced this pull request Nov 12, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
* master:
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 12, 2020
… alerts/action-groups-as-conditions

* origin/alerts/stack-alerts-public: (91 commits)
  removed import from plugin code as it causes FTR to fail
  [Advanced Settings] Introducing telemetry (elastic#82860)
  [alerts] add executionStatus to event log doc for action execute (elastic#82401)
  Add additional sources routes (elastic#83227)
  [ML] Persisted URL state for the "Anomaly detection jobs" page (elastic#83149)
  [Logs UI] Add pagination to the log stream shared component (elastic#81193)
  [Index Management] Add an index template link to data stream details (elastic#82592)
  Add maps_oss folder to code_owners (elastic#83204)
  fix truncation issue (elastic#83000)
  [Ingest Manger] Move asset getters out of registry (elastic#83214)
  make defaulted field non maybe
  Remove unused asciidoc file (elastic#83228)
  [Lens] Remove background from lens embeddable (elastic#83061)
  [Discover] Unskip flaky tests based on discover fixture index pattern (elastic#82991)
  Removing unnecessary trailing slash in CODEOWNERS
  Trying to fix CODEOWNERS again, where was a non-existent team prior (elastic#83236)
  Trying to fix CODEOWERS, missing a starting slash (elastic#83233)
  skip flaky suite (elastic#83231)
  Add enzyme rerender test helper (elastic#83208)
  Move Elasticsearch type definitions out of APM (elastic#83081)
  ...
@Dosant Dosant mentioned this pull request Nov 18, 2020
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants