Conversation
CoenWarmer
added
the
release_note:skip
| defaultMessage: 'Created', | ||
| }); | ||
|
|
||
| export const confirmModalText = ( |
There was a problem hiding this comment.
These translation functions were only used in DeleteConfirmationModal.
DeleteConfirmationModal was set up in a way where it could only delete one thing at a time, so de facto numIdsToDelete would always be 1.
Since it would only allow the deletion of 1 thing at a time, it made no sense to pass a multipleTitle.
By removing these constraints, it became possible to increase the legibility of the i18n string by writing "You can't recover {title} after deleting." and just including it in the component that used it.
| @@ -18,7 +17,9 @@ export function CenterJustifiedSpinner({ size }: Props) { | |||
| return ( | |||
| <EuiFlexGroup data-test-subj="centerJustifiedSpinner" justifyContent="center"> | |||
There was a problem hiding this comment.
This makes the loading state for this page look nicer (the spinner is no longer at the top edge of the page).
I do think we need to unify the page loaders for Overview, Rules, Rule Details, Alerts, Alert Detail into one component so there's less code to manage and more UI consistency, but that can happen in a different PR.
| }; | ||
|
|
||
| const features = ( | ||
| rule?.consumer === ALERTS_FEATURE_ID && ruleType?.producer ? ruleType.producer : rule?.consumer |
There was a problem hiding this comment.
This looks slightly magical to me, but it is unchanged from the previous implementation. Perhaps someone can illuminate what this variable means and how the way it is derived makes sense?
| multipleTitle={rule.name} | ||
| setIsLoadingState={() => setIsPageLoading(true)} | ||
| /> | ||
| {errorRule && toasts.addDanger({ title: errorRule })} |
There was a problem hiding this comment.
This is calling a method on toasts. It is not a JSX component. It should not be placed in the return of the JSX component
| onCancel={() => setRuleToDelete([])} | ||
| apiDeleteCall={deleteRules} | ||
| idsToDelete={ruleToDelete} | ||
| singleTitle={rule.name} |
There was a problem hiding this comment.
singleTitle and multipleTitle are always the same, no need for separate props
| }} | ||
| onCancel={() => setRuleToDelete([])} | ||
| apiDeleteCall={deleteRules} | ||
| idsToDelete={ruleToDelete} |
There was a problem hiding this comment.
prop is called idsToDelete implying that at some point this component was meant to be able to remove multiple things at once, but the Rule Details page can't delete multiple things and the modal (which is in the components subfolder of Rule Details so is only meant to be used here) can not either.
Perhaps this component was copied over from another place where it could delete multiple things at once.
| }} | ||
| /> | ||
| {editFlyoutVisible && | ||
| getEditAlertFlyout({ |
There was a problem hiding this comment.
Component methods exported by TriggerActionsUI are JSX components, so used JSX component syntax (<EditAlertFlyout />)
| pageHeader={{ | ||
| pageTitle: <PageTitle rule={rule} />, | ||
| bottomBorder: false, | ||
| rightSideItems: hasEditButton |
There was a problem hiding this comment.
Value of rightSideItems broken out to its own component
| <RuleAlertsSummary rule={rule} filteredRuleTypes={filteredRuleTypes} /> | ||
| </EuiFlexItem> | ||
|
|
||
| const statusMessage = isLicenseError |
There was a problem hiding this comment.
Broken out into its own function getStatusMessage
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| setEditRuleFlyoutVisible(false); | ||
| }; | ||
|
|
||
| const hasEditButton = |
There was a problem hiding this comment.
Broken out into its own hook: useIsRuleEditable
| toasts.addDanger({ title: errorRule }); | ||
| } | ||
|
|
||
| const tabs: EuiTabbedContentTab[] = [ |
There was a problem hiding this comment.
Broken out into its own component: <RuleDetailTabs />
| @@ -1,93 +0,0 @@ | |||
| /* | |||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | |||
There was a problem hiding this comment.
Renamed to <DeleteConfirmationModal />
| @@ -1,35 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Renamed to /helpers/utils.ts as it provided helper utilities more so than configuration
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
|
@CoenWarmer Does this PR still require review? |
I still think it's an improvement, but it's a little outdated now. Let me update it and I'll ping you. |
5 participants
Summary
This PR aims to refactor the Rule Details page for more maintainability and better legibility.
Functionally the page should behave exactly the same.
It is recommended to review this PR commit by commit.
Guidelines while refactoring:
pageTitle: string | component,rightSideItems: [<HeaderActions />])Part of refactoring effort of other AO pages: