[Mappings editor] Refactor field "path" format#54146
[Mappings editor] Refactor field "path" format#54146sebelga merged 8 commits intoelastic:feature/mappings-editorfrom
Conversation
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
There was a problem hiding this comment.
General
Code and UI look great! No blockers found.
- I don't think this is an issue with the code in this PR, but I was able to trigger a react warning while updating a
Texttype to anObjecttype. Could not reproduce consistently so this is a flaky one 🎉 :
This happened after clicking "Update". Looks like an issue on the EUI side while EuiComboBox was doing something in the background. I can open an issue there if you agree with this @sebelga
UX Feedback
- I can see that the icon renders nicely in this space, I'm just wondering if it's worth using this over the
>we use everywhere else. To me it also feels a bit like amapis happening, i.e., we are convertingtest1totest2by way of a mapping function #algebraic.
| * The <EuiCode /> component expect the children provided to be a string (html). | ||
| * This component allows both string and JSX element | ||
| * | ||
| * TODO: Open PR on eui repo to allow both string and React.Node to be passed as children of <EuiCode /> |
There was a problem hiding this comment.
Do we have an issue open somewhere for this?
There was a problem hiding this comment.
No, it is still a TODO. Going through the TODOs might be a good thing to do during the extra week at the end of the cycle.
| @@ -103,7 +103,7 @@ const replaceAliasPathByAliasId = ( | |||
| Object.entries(byId).forEach(([id, field]) => { | |||
| if (field.source.type === 'alias') { | |||
| const aliasTargetField = Object.values(byId).find( | |||
There was a problem hiding this comment.
nit: just saw this now, but Object.values(byId) can be calculated once outside of the .forEach.
There was a problem hiding this comment.
This is actually not a nit, it would be a NNNIIITTT if that word existed 😄 It caused a big performance issue that I saw and fixed in #54224
Great catch though! 👍
cjcenizal
left a comment
There was a problem hiding this comment.
Nice work, Seb! I didn't test locally, but the code and screenshots LGTM. I agree with JL that it makes more sense to be consistent with the use of greater-than as a delimiter, rather than introduce the EuiIcon in the flyout header.
|
Thanks for the review @jloleysens and @cjcenizal
I also saw randomly this error coming up in the console and it did seem like a Eui bug (updating a state after the component is unmounted). Before opening an issue on their side I'd like to be able to reproduce it first so we are 100% sure it is not coming from us.
OK, I will revert to |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
This PR refactors the state format for a field
pathfrom a stringmyObject.propto an array of string['myObject', 'prop'].This allows us to render the path as we wish.
For the dropdown where we render the label of the
aliaspath to point to, I decided to keep the>delimiter as having that char in the naming is probably an edge case.Fixes #53894