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

[Dashboard] Fix breadcrumb update icon does not working when clicked #192240

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

viajes7
Copy link
Contributor

@viajes7 viajes7 commented Sep 6, 2024

Summary

Fixes #186132

In the header_breadcrumbs.test.tsx, forces the last breadcrumb inactivity. So onClick did not take effect finally.

So, I'm considering putting onClick on the update icon.

PS: This is my first time to contribute code to kibana. If there is anything that needs to be modified, please give me some advice and I will try my best.

Copy link

cla-checker-service bot commented Sep 6, 2024

💚 CLA has been signed

@Heenawter Heenawter added release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Sep 6, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@Heenawter
Copy link
Contributor

buildkite test this

@Heenawter
Copy link
Contributor

@elasticmachine run buildkite/docs-build-pr

Comment on lines +182 to +189
{dashboardTitle}
<EuiIcon
size="s"
type="pencil"
className="dshTitleBreadcrumbs__updateIcon"
onClick={() => dashboard.showSettings()}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

@cqliu1 @andreadelrio How do we feel about this change? The last breadcrumb is enforced to be unclickable, and so the onClick is literally being stripped out when building the top nav (see #166785). This seems like a decent workaround to me (just making the icon clickable) but I'm interested in ya'lls thoughts 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with having just the pencil icon be clickable. That's how I've seen other apps handle it

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me as well!

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Code review + local test. Pencil icon now opens the Dashboard settings menu, and it works as expected. Thanks so much for your contribution 🎉

@viajes7
Copy link
Contributor Author

viajes7 commented Sep 9, 2024

@elasticmachine merge upstream

1 similar comment
@viajes7
Copy link
Contributor Author

viajes7 commented Sep 10, 2024

@elasticmachine merge upstream

@viajes7
Copy link
Contributor Author

viajes7 commented Sep 10, 2024

Hi @Heenawter, I noticed that a member of the elastic/kibana-design team is also needed to review the code.
Can you help me contact the relevant members? Thanks.

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design changes LGTM, thanks @viajes7 !

@viajes7
Copy link
Contributor Author

viajes7 commented Sep 10, 2024

@elasticmachine merge upstream

@viajes7
Copy link
Contributor Author

viajes7 commented Sep 10, 2024

@Heenawter Hi, The pipeline seems to be stuck, do you have any suggestions?

@Heenawter
Copy link
Contributor

buildkite test this

@Heenawter
Copy link
Contributor

@elasticmachine run buildkite/docs-build-pr

@Heenawter
Copy link
Contributor

buildkite test this

@Heenawter
Copy link
Contributor

@elasticmachine run buildkite/docs-build-pr

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #7 / TableSearch renders with initial value correctly

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 457.1KB 457.3KB +156.0B

History

  • 💛 Build #232493 was flaky b223701f389ddfb1186e60251dd79f189718c2dd

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

@Heenawter Heenawter merged commit 1a51180 into elastic:main Sep 10, 2024
21 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 💝community release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Clicking on dashboard title breadcrumb in edit mode doesn't open dashboard settings
7 participants