[App Search] Remaining Result Settings logic and routes #94947
Conversation
JasonStoltz
added
release_note:skip
|
@elasticmachine merge upstream |
| @@ -90,6 +114,15 @@ export const ResultSettingsLogic = kea<MakeLogicType<ResultSettingsValues, Resul | |||
| resetAllFields: () => true, | |||
| updateField: (fieldName, settings) => ({ fieldName, settings }), | |||
| saving: () => true, | |||
| clearRawSizeForField: (fieldName) => fieldName, | |||
There was a problem hiding this comment.
Even when I'm only passing a single argument, I've been wrapping it like clearRawSizeForField: (fieldName) => ({fieldName}) because I like the consistency, and also when I realize I need to add another argument later. This is fine, just wanted to mention my approach.
There was a problem hiding this comment.
It's definitely the better approach. Kea recommends the same thing:
Actions are functions that take whatever arguments you choose and return a payload. This payload should always be an object: (amount) => ({ amount }).
There was a problem hiding this comment.
I was just trying to migrate this, so I moved it over as is.
There was a problem hiding this comment.
I had to make changes anyway, so I updated this: aa858df
| Object.entries(serverResultFields).reduce( | ||
| (acc: ServerFieldResultSettingObject, [fieldName, resultSetting]) => { | ||
| if (resultSetting.raw || resultSetting.snippet) { | ||
| acc[fieldName] = resultSetting; | ||
| } | ||
| return acc; | ||
| }, | ||
| {} | ||
| ), |
There was a problem hiding this comment.
Functional programming is funny! Here's an alternative approach:
Object.entries(serverResultFields)
.filter(([_, resultSetting]) => resultSetting.raw || resultSetting.snippet)
.reduce((acc, [fieldName, resultSetting]) => {...acc, [fieldName]: resultSetting}, {})
I don't think you need to change it, they both accomplish the same thing and are about as readable, just wanted to flex a little 💪
| actions.updateField(fieldName, omit(values.resultFields[fieldName], ['snippetSize'])); | ||
| }, | ||
| toggleRawForField: (fieldName: string) => { | ||
| // We cast this b/c it could be an empty object, which we can still treat as a FieldResultSetting safely |
There was a problem hiding this comment.
I'm not a huge fan of stuff like "b/c" in comments because it can be confusing to non-native English speakers.
| export const areFieldsEmpty = (fields: FieldResultSettingObject) => { | ||
| const anyNonEmptyField = Object.values(fields).find((resultSettings) => { | ||
| return !isEmpty(resultSettings); | ||
| }); | ||
| return !anyNonEmptyField; | ||
| }; |
There was a problem hiding this comment.
You could probably re-write this to use a for...in loop, which would allow you to short-circuit immediately if you founnd a non-empty field, but we're not talking about 1,000,000 fields here.
There was a problem hiding this comment.
True. .find will return the first match it finds in an array so I believe it is equally efficient, fwiw.
|
Meant to include this w/ approval: I left some non-blocking thoughts in a few places, this looks fine to me to merge |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
) Co-authored-by: Jason Stoltzfus <[email protected]>
Summary
This PR migrates the remaining logic and server routes for Result Settings.
This PR is probably best reviewed commit by commit. Server routes are not added in a single commit, they are added along with the commit that adds a listener that requires them.
This logic is migrated as is from ent-search. I believe as discussed in the last PR there are opportunities for refactor. Please note them here, and if it's alright, I'd like to address them on a case by case basis as I see how the UI is actually using these methods. For example, I think we could remove redundant server and client fields state. We also may be able to convert these custom "confirmation" modals to regular widow.confirms, like we do elsewhere in the app. However, I'm not comfortable doing this until I can actually test this end to end with the UI in place.
Checklist
Delete any items that are not applicable to this PR.
For maintainers