[ML] Transforms: Single Column Wizard. #64436
Merged
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
|
Pinging @elastic/ml-ui (:ml) |
x-pack/plugins/transform/public/app/sections/create_transform/components/wizard/wizard.tsx
Outdated
Show resolved
Hide resolved
| <EuiFormRow hasEmptyLabelSpace> | ||
| <EuiFlexGroup alignItems="center" justifyContent="spaceBetween"> | ||
| <EuiFlexItem grow={false}> | ||
| <AdvancedPivotEditorSwitch {...stepDefineForm} /> |
There was a problem hiding this comment.
Could the switch be moved to the top of this section? Currently it is aligned with the 'Group by' control which gives the impression it is only for editing the group_by part of the pivot.
There was a problem hiding this comment.
As discussed, I'll leave it as is for now, moving it up looks misaligned (see screenshot), moving it even further up would result in additional vertical whitespace
...s/transform/public/app/sections/create_transform/components/step_define/step_define_form.tsx
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
x-pack/plugins/ml/public/application/components/data_grid/data_grid.tsx
Outdated
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
darnautov
reviewed
May 4, 2020
...form/public/app/sections/create_transform/components/source_search_bar/source_search_bar.tsx
Outdated
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
Comment on lines
59
to
68
| return aggListNameSplit.some(aggListNamePart => { | ||
| aggListNameCheck = | ||
| aggListNameCheck === undefined ? aggListNamePart : `${aggListNameCheck}.${aggListNamePart}`; | ||
| if (aggListNameCheck === aggName) { | ||
| conflicts.push( | ||
| i18n.translate('xpack.transform.stepDefineForm.nestedAggListConflictErrorMessage', { | ||
| defaultMessage: `Couldn't add configuration '{aggName}' because of a nesting conflict with '{aggListName}'.`, | ||
| values: { aggName, aggListName }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
I don't think it's a good practice when some method contains a side effect, in this case, push to the collection. Consider using for..of with break statement
There was a problem hiding this comment.
Good find, this was some older code I copied, changed it in 9d6f488.
There was a problem hiding this comment.
just checked, please use loop label names, check https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/break for examples
darnautov
reviewed
May 4, 2020
Comment on lines
83
to
93
| return groupByListNameSplit.some(groupByListNamePart => { | ||
| groupByListNameCheck = | ||
| groupByListNameCheck === undefined | ||
| ? groupByListNamePart | ||
| : `${groupByListNameCheck}.${groupByListNamePart}`; | ||
| if (groupByListNameCheck === aggName) { | ||
| conflicts.push( | ||
| i18n.translate('xpack.transform.stepDefineForm.nestedGroupByListConflictErrorMessage', { | ||
| defaultMessage: `Couldn't add configuration '{aggName}' because of a nesting conflict with '{groupByListName}'.`, | ||
| values: { aggName, groupByListName }, | ||
| }) |
There was a problem hiding this comment.
same as the comment above
darnautov
reviewed
May 4, 2020
...ugins/transform/public/app/sections/create_transform/components/step_define/hooks/use_agg.ts
Outdated
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
...nsform/public/app/sections/create_transform/components/step_define/hooks/use_pivot_config.ts
Outdated
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
...nsform/public/app/sections/create_transform/components/step_define/hooks/use_pivot_config.ts
Outdated
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
...ns/transform/public/app/sections/create_transform/components/step_define/common/constants.ts
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
...rm/public/app/sections/create_transform/components/step_define/hooks/use_step_define_form.ts
Outdated
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
...rm/public/app/sections/create_transform/components/step_define/hooks/use_step_define_form.ts
Show resolved
Hide resolved
darnautov
reviewed
May 4, 2020
...ransform/public/app/sections/create_transform/components/step_define/step_define_summary.tsx
Outdated
Show resolved
Hide resolved
peteharverson
approved these changes
May 4, 2020
|
Latest changes LGTM ⚡ |
darnautov
approved these changes
May 4, 2020
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
walterra
added a commit
that referenced
this pull request
May 5, 2020
Rearranges the layout of the transform wizard pivot configuration step into a single-column. This allows us to have the data grids for source index and pivot preview having the full width. The advanced editors for source query and pivot configuration also cover a wider width.
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
<StepDefineForm />. The code has been rearranged into individual components and hooks. Some optimizations have been added, for exampleReact.memo()including a custom props compare function avoid a rerender on unrelated changes to the data grids. This optimization is also now available on the analytics results pages.Checklist
Delete any items that are not applicable to this PR.
For maintainers