[App Search] API Logs: Add ApiLogsTable and NewApiEventsPrompt components #96008
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cee-chen
added
Feature:Plugins
release_note:skip
cee-chen
commented
Mar 31, 2021
Comment on lines
+59
to
+61
| <EuiPageContent hasBorder> | ||
| <EuiPageContentBody> | ||
| <EuiFlexGroup gutterSize="m" alignItems="center" responsive={false} wrap> |
There was a problem hiding this comment.
Everything inside here is mostly the same as before, I just added EuipageContent and EuiPageContentBody wrappers - I recommend viewing with hidden whitespace changes for easier diffing
| @@ -0,0 +1,3 @@ | |||
| .apiLogDetailButton { | |||
| height: $euiSizeL !important; | |||
There was a problem hiding this comment.
With:
Without:
I was trying to maintain the more compact rows from our standalone UI (as well as matching our other tables), but if we think that's overkill I can get rid of it.
FWIW without the ! important I need 3 levels of nesting to override EUI's styles, so I took the lazy way out 😬
There was a problem hiding this comment.
If it were me, I'd say just let EUI do it's thing. However, if there's something unique about this table that is forcing the rows to be taller than usual, then I'd say this is a worthwhile change.
It's up to you though. Is it worthwhile to drop a comment in the CSS about why you have added that or nah?
To be clear, I'm good either way, I'll approve it regardless.
There was a problem hiding this comment.
The first screenshot more closely matches the row heights in all their examples in their docs. The main difference is that their buttons are all icon buttons and not empty buttons 🤷 It does seem odd that EUI wouldn't have any affordance OOTB for EuiEmptyButton matching the same line height as an EuiLink.
A comment makes a ton of sense! I'll add one here in a bit.
Comment on lines
+2
to
+3
| padding: $euiSizeXS; | ||
| padding-left: $euiSizeS; |
There was a problem hiding this comment.
FYI: without this custom padding, the height of the panel exceeds the height of the row height and causes the page to bounce around awkwardly, so I added it as a polish item
Comment on lines
+14
to
+21
| export const getStatusColor = (status: number) => { | ||
| let color = ''; | ||
| if (status >= 100 && status < 300) color = 'secondary'; | ||
| if (status >= 300 && status < 400) color = 'primary'; | ||
| if (status >= 400 && status < 500) color = 'warning'; | ||
| if (status >= 500) color = 'danger'; | ||
| return color; | ||
| }; |
There was a problem hiding this comment.
I basically just copied this util over as-is from the standalone UI. Def. open to thoughts or suggestions if you have any changes you'd like to see
...rise_search/public/applications/app_search/components/api_logs/components/api_logs_table.tsx
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
+ add EuiPageContent wrapper (missed this originally)
JasonStoltz
reviewed
Apr 1, 2021
| } | ||
| > | ||
| TODO: API Logs Table | ||
| {/* <ApiLogsTable hidePagination={true} /> */} | ||
| <ApiLogsTable /> |
There was a problem hiding this comment.
It's really cool that we were able to reuse this in both places.
JasonStoltz
reviewed
Apr 1, 2021
Comment on lines
+94
to
+95
| wrapper.find('[data-test-subj="ApiLogsTableDetailsButton"]').first().simulate('click'); | ||
| // TODO: API log details flyout |
There was a problem hiding this comment.
So it looks like you intend to test the flyout opening in the primary renders test?
There was a problem hiding this comment.
Oops, just realized I forgot to respond to this - sorry Jason! Yeah the tables are a bit weird testing-wise and mount is a somewhat-expensive render so I figured just throw it in the main test. I can move it out though if we prefer
Comment on lines
+14
to
+21
| export const getStatusColor = (status: number) => { | ||
| let color = ''; | ||
| if (status >= 100 && status < 300) color = 'secondary'; | ||
| if (status >= 300 && status < 400) color = 'primary'; | ||
| if (status >= 400 && status < 500) color = 'warning'; | ||
| if (status >= 500) color = 'danger'; | ||
| return color; | ||
| }; |
cee-chen
commented
Apr 1, 2021
...search/public/applications/app_search/components/api_logs/components/api_logs_table.test.tsx
Outdated
Show resolved
Hide resolved
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Apr 2, 2021
…ents (elastic#96008) * Set up getStatusColor util for upcoming EuiHealth components * Add ApiLogsTable component * Add NewApiEventsPrompt component * Update ApiLogs view with new components + add EuiPageContent wrapper (missed this originally) * Update EngineOverview with new components * PR feedback: Comments + mock FormattedRelative * Fix type error
kibanamachine
added a commit
that referenced
this pull request
Apr 2, 2021
…ents (#96008) (#96127) * Set up getStatusColor util for upcoming EuiHealth components * Add ApiLogsTable component * Add NewApiEventsPrompt component * Update ApiLogs view with new components + add EuiPageContent wrapper (missed this originally) * Update EngineOverview with new components * PR feedback: Comments + mock FormattedRelative * Fix type error Co-authored-by: Constance <[email protected]>
6 tasks
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/maps.maps app "before all" hook in "maps app"Standard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/discover/feature_controls/discover_security·ts.discover feature controls discover feature controls security "before all" hook in "discover feature controls security"Standard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/discover/feature_controls/discover_security·ts.discover feature controls discover feature controls security "after all" hook in "discover feature controls security"Standard OutStack Traceand 16 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |
|
lmfao what??? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Second-to-last PR in the series. The next one adds the "Details" view functionality.
Screencaps
Engine Overview:
QA
Checklist