-
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
[Event annotations] Individual annotation editing from library #163346
[Event annotations] Individual annotation editing from library #163346
Conversation
…emon/kibana into add-individual-annotation-editing
3ea8e3b
to
36bc171
Compare
…emon/kibana into add-individual-annotation-editing
0014cf0
to
8fe5a47
Compare
New functional tests passed flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3134#_ |
}); | ||
}); | ||
|
||
describe.skip('data view switching', () => { |
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.
This fails in CI but passes locally. I decided to address this as part of #159053
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.
Thanks so much again, @drewdaemon. This looks wonderful. I've left you a few final comments below, but nothing worth holding up this PR further. Assuming you're able to review them, I'm approving now.
-
If all annotation dimension items are removed, there is some extra spacing at the top of the container, above the "Add annotation" button. This is appears due to the top margin of the button. Can you style it so that this extra space doesn't appear when dimension items are absent?
-
Can you remove the placeholder text for the color picker altogether? As we're now restoring the default hex value on blur of the field, this no longer needs to be there.
...isting/public/components/group_editor_flyout/group_editor_controls/group_editor_controls.tsx
Show resolved
Hide resolved
src/plugins/event_annotation_listing/public/components/group_editor_flyout/group_preview.tsx
Outdated
Show resolved
Hide resolved
...isting/public/components/group_editor_flyout/group_editor_controls/group_editor_controls.tsx
Show resolved
Hide resolved
...isting/public/components/group_editor_flyout/group_editor_controls/group_editor_controls.tsx
Outdated
Show resolved
Hide resolved
src/plugins/event_annotation_listing/public/components/group_editor_flyout/group_preview.tsx
Outdated
Show resolved
Hide resolved
...isting/public/components/group_editor_flyout/group_editor_controls/group_editor_controls.tsx
Outdated
Show resolved
Hide resolved
...isting/public/components/group_editor_flyout/group_editor_controls/group_editor_controls.tsx
Outdated
Show resolved
Hide resolved
...ugins/event_annotation_listing/public/components/group_editor_flyout/group_editor_flyout.tsx
Outdated
Show resolved
Hide resolved
...ugins/event_annotation_listing/public/components/group_editor_flyout/group_editor_flyout.tsx
Outdated
Show resolved
Hide resolved
...ugins/event_annotation_listing/public/components/group_editor_flyout/group_editor_flyout.tsx
Outdated
Show resolved
Hide resolved
<EuiTitle size="xs"> | ||
<h2 id={flyoutHeadingId}> |
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.
Again, a subjective decision, but would it be better to wrap this EuiTitle
component and h2
element around the title text only (rather than the title and the back button)?
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.
I see where you're coming from, but I like it this way since we don't have to duplicate the elements between the two titles in the markup
…emon/kibana into add-individual-annotation-editing
Great find! This turned out to be a small oversight on my part (instead of an embeddable issue as I originally feared). Fixed in 50ea86e |
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.
Bug fixed, tested again the basic scenarios, everything seems to work as expected 👏
LGTM, great job 💯
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
miscellaneous assets size
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
Summary
Resolve #158774
Part of #159053
Known issues
Responsive layoutProposal: don't optimize for mobileChecklist