[ILM] Hide rollover controls in advanced #84198
Conversation
jloleysens
added
release_note:enhancement
Feature:ILM
v8.0.0
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Hi @jloleysens , nice work improving ILM interface! I really like having less controls in the form, that would make this form much easier to navigate 👍
For rollover controls, I would like to see default values filled out visually in the form to understand better (though they are written down in the description). Also, I assumed that disabling customize option will save rollover as enabled with default values (basically resetting anything I changed on the right side), but the form 'remembered' that I disabled rollover toggle even after the toggle disappeared.
For switches you are probably waiting on design review, so one UI suggestion to consider could be to move the switch on the same line as the row title (screenshot attached). I think that duplicating 'Force merge' in the title and 'Force merge data' in the switch is a bit superfluous.
There are some comments I left directly in the code. Let me know what you think! :)
| export const isUsingDefaultRolloverConfig = (hotPhase?: SerializedHotPhase) => | ||
| Boolean( | ||
| hotPhase?.actions.rollover?.max_size === '50gb' && | ||
| hotPhase.actions.rollover.max_age === '50d' && |
There was a problem hiding this comment.
The default value for max_age on line 12 is 30d whereas this function checks for 50d. Wouldn't it be less error prune to compare with const? For example: hotPhase?.actions.rollover?.max_age === ROLLOVER_DEFAULT.max_age
| return ( | ||
| <EuiAccordion | ||
| id="advancedSettingsRollover" | ||
| buttonContent={showAdvancedSettings ? 'Hide advanced settings' : 'Show advanced settings'} |
There was a problem hiding this comment.
There is missing 'i18n' here
| } | ||
| switchProps={{ | ||
| path: hasConfiguredRolloverPath, | ||
| label: 'Customize rollover', |
There was a problem hiding this comment.
'i18n' missing here too, also line 50 (title prop) and line 51 (description prop).
| ); | ||
| if (showErrorCallout !== showEmptyRolloverFieldsError) { | ||
| setShowEmptyRolloverFieldsError(showErrorCallout); | ||
| } |
There was a problem hiding this comment.
If I understand correctly, this hook sets the value of showEmptyRolloverFieldsError state if there is ROLLOVER_EMPTY_VALIDATION error. I think the comparison on line 116 is not needed, as the setter would handle the same value automatically, right? So it could be something like:
const hasEmptyRolloverError = field.errors.some(
(e) => e.validationType === ROLLOVER_EMPTY_VALIDATION
);
setShowEmptyRolloverFieldsError(hasEmptyRolloverError);
WDYT?
| interface SwitchProps extends Omit<EuiSwitchProps, 'checked' | 'onChange'> { | ||
| initialValue?: boolean; | ||
| path?: string; | ||
| keepChildrenMounted?: boolean; |
There was a problem hiding this comment.
I believe the prop keepChildrenMounted is duplicated and the one in Props type is actually used in the code.
| onChange={(e) => { | ||
| const nextValue = e.target.checked; | ||
| if (typeof controlledValue !== 'boolean') { | ||
| setUncontrolledValue(nextValue); |
There was a problem hiding this comment.
controlledValue should be undefined since we don't have path (line 53 and line 41), so my guess is that we don't need the typeof check.
|
@yuliacech Thanks for reviewing the code! This was actually not yet ready for review by ES-UI, which is why I did not explicitly add a reviewer, but I did want to get early design review. In future I will make the PR a draft 👍 |
|
I think this is looking good. Just a few points:
|
Summary
Improve the ILM form by moving rollover to an advanced section in the hot phase. Also refactor the description fields to have their enable toggles on the right under their descriptions.
Note all copy is temporary and still needs to be reviewed!
Gif
Screenshots
moved form field toggles to the right under their descriptions:
How to test
This change is (mostly) aesthetic. ILM policy creation should work exactly as before. To test, create a new policy editing all fields and check the JSON in the show request flyout.
Also, load an existing policy in the UI and check that everything is as expected.
Checklist
Delete any items that are not applicable to this PR.
For maintainers