-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Remove visible title in workspace panel #82234
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Pinging @elastic/kibana-app (Team:KibanaApp) |
Thanks for working on this @lykims . I checked the PR and it seems like the empty header is still adding margin to the top - this space should be used by the chart: Also there are now unused css classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the PR and there is some wasted whitespace in place of the header
Hi @flash1293! Good observation! If the CSS classes If the UI element <EuiPageContent className="lnsWorkspacePanelWrapper">
{!emptyExpression || title ? (
<EuiScreenReaderOnly>
<h1 data-test-subj="lns_ChartTitle">
{title ||
i18n.translate('xpack.lens.chartTitle.unsaved', { defaultMessage: 'Unsaved' })}
</h1>
</EuiScreenReaderOnly>
) : (
<EuiScreenReaderOnly>
<h1 data-test-subj="lns_ChartTitle">
{title ||
i18n.translate('xpack.lens.chartTitle.unsaved', { defaultMessage: 'Unsaved' })}
</h1>
</EuiScreenReaderOnly>
)}
... Then, the workspace part could simply look like: <EuiPageContent className="lnsWorkspacePanelWrapper">
<EuiScreenReaderOnly>
<h1 data-test-subj="lns_ChartTitle">
{title ||
i18n.translate('xpack.lens.chartTitle.unsaved', { defaultMessage: 'Unsaved' })}
</h1>
</EuiScreenReaderOnly>
... and the prop What do you think? |
@lykims Sounds good to me!
Yeah, removing the classes and removing the margin are two separate things I just put in a single comment |
@flash1293 I pushed new changes to be reviewed. |
Thanks @lykims , I will take a look later today 👍 |
@elasticmachine merge upstream |
Jenkins, test this |
@lykims Seems like there's a little additonal cleanup to do:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for leaning into the dashboard-first/default-no-title approach; breadcrumb seems sufficient in this case.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in Chrome and Firefox and looks great, thanks for this!
Jenkins, test this. |
💚 Build SucceededMetrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
Summary
Resolves #81979.
Checklist
Delete any items that are not applicable to this PR.
For maintainers