-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Migrate Index Management to new solutions nav #101548
Migrate Index Management to new solutions nav #101548
Conversation
88e1c24
to
d2a446f
Compare
fc128ce
to
a13c70c
Compare
a13c70c
to
ece3512
Compare
@@ -141,13 +145,13 @@ export const ComponentTemplateList: React.FunctionComponent<Props> = ({ | |||
let content: React.ReactNode; | |||
|
|||
if (isLoading) { | |||
content = ( | |||
<SectionLoading data-test-subj="sectionLoading"> | |||
return ( |
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 know it looks weird but this is deliberate. I think we need to go back and rearchitect our apps later for consistent patterns both across apps and even within each app.
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've never been a big fan of this content
var declared on top and assigned down.
Can we write this slightly differently, with simply an early return?
if (isLoading) {
return (
<PageLoading data-test-subj="sectionLoading">
<FormattedMessage
id="xpack.idxMgmt.home.componentTemplates.list.loadingMessage"
defaultMessage="Loading component templates…"
/>
</PageLoading>
);
}
let content: React.ReactNode;
if (data?.length) {
...
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.
We could but it would require duplicating the wrapping <div className={APP_WRAPPER_CLASS} data-test-subj="componentTemplateList">
element in several places. I think we can find a more elegant solution later when we rearchitect our apps to adhere to a consistent pattern, so I'm going to defer this for now.
On second reading it sounds like you're advocating for moving the content
var so it isn't declared before the early return, which SGTM.
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.
It all goes to down to preferences but I never have to use this pattern of declaring an empty content
and assign it in multiple place to finally render it down. I much prefer early return pattern
const SomeComponent = () => {
if (this) {
return <MyComp />;
}
if (that) {
return <MyOtherComp />;
}
const renderMainContent = () => {
if (someFlag) {
return <div>Early return</div>;
}
return <div>Finally</div>;
}
// otherwise
return <div className="someClassWeWantEverywhere">{renderMainContent()}</div>
}
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.
Ah, I see what you're saying. Yes I prefer that, too. We'll apply this in the future when we rearchitect our apps to adhere to a consistent pattern.
); | ||
} else if (!hasTemplates) { | ||
content = ( | ||
<EuiEmptyPrompt |
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 empty prompt didn't originally offer the user any actions, so I added those and updated the copy to mirror our other empty prompts.
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.
Nice! 👍
@@ -19,6 +20,6 @@ export const sendRequest = (config: SendRequestConfig): Promise<SendRequestRespo | |||
return _sendRequest(httpService.httpClient, config); | |||
}; | |||
|
|||
export const useRequest = <T = any>(config: UseRequestConfig) => { | |||
return _useRequest<T>(httpService.httpClient, config); | |||
export const useRequest = <T = any, E = Error>(config: UseRequestConfig) => { |
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 allows us to pass API errors to PageError
without a TS error.
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
@@ -183,11 +187,22 @@ export const ComponentTemplateList: React.FunctionComponent<Props> = ({ | |||
} else if (data && data.length === 0) { | |||
content = <EmptyPrompt history={history} />; | |||
} else if (error) { | |||
content = <LoadError onReloadClick={resendRequest} />; |
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 removed this state because it didn't surface the underlying error, offered a "Try again" button which I don't think will be helpful in the typical error scenario, and was inconsistent with the other error states in this PR.
5c26e49
to
fe9b346
Compare
* Add PageLoading component to es_ui_shared.
* Refactor index table component tests.
* Add missing error reporting to get all templates API route handler.
…_shared SectionLoading elsewhere.
fe9b346
to
a4f510a
Compare
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.
Great job @cjcenizal ! Tested locally and works as expected. I left a few non blocking comments in the code.
import React from 'react'; | ||
import { EuiEmptyPrompt, EuiLoadingSpinner, EuiText, EuiPageContent } from '@elastic/eui'; | ||
|
||
interface Props { |
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.
nit: you don't need to declare this interface. React.FunctionComponent
already declare the children
prop for us.
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! Fixed.
// but it looks like we're aware of how many Redux actions are dispatched in response to user interaction, | ||
// so we "time" our assertion based on how many Redux actions we observe. This is brittle because it | ||
// depends upon how our UI is architected, which will affect how many actions are dispatched. | ||
// Expect this to break when we rearchitect the UI. |
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'd rather put: "Expect to completely rewrite those tests when we rearchitect the UI"! 😄 This would be one of our tech debt. Actually I'd first rewrite the tests and follow our CIT best practices (no testing of implementation details!) and only after I would rearchitect the UI.
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.
Haha yep! I'm just marking the landmine with this comment, not prescribing how it should be addressed. I think an issue is a better place for that (more discoverable and collaborative). I'm going to raise this as a tech debt item we should address.
snapshot(namesText(rendered)); | ||
}); | ||
test('should show more when per page value is increased', () => { | ||
|
||
test('should show more when per page value is increased', async () => { |
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.
Are we testing the EUI table component here?
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.
The indices table is implemented using individual EuiTable
, EuiTablePagination
, EuiTableRowCell
, etc components. So unfortunately, no, this isn't testing EUI. It's testing our table implementation which EUI now provides an abstraction over with EuiBasicTable
. More tech debt! 🎉
@@ -166,7 +166,7 @@ describe('<ComponentTemplateList />', () => { | |||
|
|||
expect(exists('componentTemplatesLoadError')).toBe(true); | |||
expect(find('componentTemplatesLoadError').text()).toContain( | |||
'Unable to load component templates. Try again.' | |||
'Error loading component templatesInternal server error' |
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.
It seems that a space and a dot is missing after "templates".
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.
Nice spot! This is not a bug though, just an oddity due to the way the DOM API provides text
values of child elements by concatenating each child element's text value without any delimiter. So it's smashing together the title element "Error loading component templates" with the description element "Internal server error". Will add a comment to clarify.
@@ -141,13 +145,13 @@ export const ComponentTemplateList: React.FunctionComponent<Props> = ({ | |||
let content: React.ReactNode; | |||
|
|||
if (isLoading) { | |||
content = ( | |||
<SectionLoading data-test-subj="sectionLoading"> | |||
return ( |
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've never been a big fan of this content
var declared on top and assigned down.
Can we write this slightly differently, with simply an early return?
if (isLoading) {
return (
<PageLoading data-test-subj="sectionLoading">
<FormattedMessage
id="xpack.idxMgmt.home.componentTemplates.list.loadingMessage"
defaultMessage="Loading component templates…"
/>
</PageLoading>
);
}
let content: React.ReactNode;
if (data?.length) {
...
@@ -292,7 +292,7 @@ export const TemplateForm = ({ | |||
return ( | |||
<> | |||
{/* Form header */} | |||
{title} | |||
<EuiPageHeader pageTitle={<span data-test-subj="pageTitle">{title}</span>} bottomBorder /> |
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.
Should we ask the EUI team to add a generic data-test-subj
for us so we don't have to provide everywhere a <span>
for the title? We should only have one page header per page right?
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.
Good idea. They already have a generic data-test-subj
, but we're excluding the work of migrating our code and tests over to it from the scope of this work due to time pressure.
@@ -166,16 +170,16 @@ export const DataStreamList: React.FunctionComponent<RouteComponentProps<MatchPa | |||
|
|||
if (isLoading) { | |||
content = ( | |||
<SectionLoading> | |||
<PageLoading> |
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.
Same here. It seems we did some copy/paste of this pattern 😊
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'm not sure what you're suggesting here, can you clarify? If it's the same comment as #101548 (comment) then I'll defer to this later when we rearchitect for a consistent pattern across apps.
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.
Yes it was the same comment about early return. It's OK to leave it as is, goes down to preferences. 👍
if (!hasContent) { | ||
const renderNoContent = () => { | ||
if (indicesLoading) { | ||
return ( |
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.
Returning early here again would make the code easier to read.
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.
Do you mean converting else if
to if
? From my reading of the code, it looks like it's already returning early.
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.
Yes, once you have an early return there is no need of else if
. Just code styling thing, nothing important.
if (something) {
return true;
}
return false;
// instead of
if (something) {
return true;
} else {
return false;
}
if (isLoading) { | ||
return ( | ||
<SectionLoading> | ||
// Track component loaded |
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.
nit: wouldn't it be "templates" loaded?
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.
Hmm this is pre-existing so I can't be certain, but it looks like this code will execute when the component mounts, regardless of whether the templates are loading, loaded, or failed to load. So I have to assume that by "loaded" the original author meant "mounted". Fixed.
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.
No I meant that the tracking. UIM_TEMPLATE_LIST_LOAD
is for "index template loaded" not "component templates loaded" 😊
); | ||
} else if (!hasTemplates) { | ||
content = ( | ||
<EuiEmptyPrompt |
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.
Nice! 👍
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Migrate index template and component template wizard pages to new nav. * Convert index templates and component templates pages to new nav. * Convert indices and data streams pages to new nav. * Add PageLoading component to es_ui_shared. * Refactor index table component tests. * Add missing error reporting to get all templates API route handler. # Conflicts: # x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts
* Migrate index template and component template wizard pages to new nav. * Convert index templates and component templates pages to new nav. * Convert indices and data streams pages to new nav. * Add PageLoading component to es_ui_shared. * Refactor index table component tests. * Add missing error reporting to get all templates API route handler. # Conflicts: # x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts
Partially addresses #100748
Notable changes:
PageError
component'serror
prop optional so we could use it to surface errors like missing permissions, which don't report an ES error.PageLoading
component.Screenshots
Indices
Data streams
Index templates
List
Wizard
Component templates
List
Wizard