[Curation] Add Result Manually flyout/search #94887
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
- not really sure which API endpoint to use, hedging my bets
- fairly simple, mostly concerned with flyout behavior & search query - could likely be reused (or replaced with??) query tester logic in the future
cee-chen
added
Feature:Plugins
release_note:skip
- with custom isPromoted / isHidden logic & actions
cee-chen
commented
Mar 18, 2021
|
|
||
| router.get( | ||
| { | ||
| path: '/api/app_search/engines/{engineName}/curation_search', |
There was a problem hiding this comment.
I have no idea if this is a good API route name, sorry y'all 😬 Def feel free to give suggestions, I was worried curations/search made it seem like you're searching for curations rather than searching for documents 😕
| onSearch({ results }: { results: Result[] }): { results: Result[] }; | ||
| } | ||
|
|
||
| export const AddResultLogic = kea<MakeLogicType<AddResultValues, AddResultActions>>({ |
There was a problem hiding this comment.
This new logic file is pretty simple - I'm thinking we could reuse it for the query tester flyout which is super similar, or potentially just straight up convert the AddResultFlyout into the QueryTesterFlyout (just passing custom actions)
|
|
||
| expect(http.get).toHaveBeenCalledWith( | ||
| '/api/app_search/engines/some-engine/curation_search', | ||
| { query: { query: 'hello world' } } |
There was a problem hiding this comment.
This made me double take initially but it's legit lol. The first query is the payload format our server wants, and the 2nd query is param key we use for searching
Comment on lines
+86
to
+94
| it('sets searchResults & dataLoading to false', () => { | ||
| mount({ searchResults: [], dataLoading: true }); | ||
|
|
||
| AddResultLogic.actions.onSearch(MOCK_SEARCH_RESPONSE); | ||
|
|
||
| expect(AddResultLogic.values).toEqual({ | ||
| ...DEFAULT_VALUES, | ||
| searchResults: MOCK_SEARCH_RESPONSE.results, | ||
| dataLoading: false, |
There was a problem hiding this comment.
Curse you @JasonStoltz, I really want the helper you described in #94769 (comment) now 😆
cee-chen
commented
Mar 18, 2021
| searchQuery: [ | ||
| '', | ||
| { | ||
| search: (_, { query }) => query, |
There was a problem hiding this comment.
Oh yeah, one more question for y'all!! I was wondering about this when I was recording the screen capture for the PR, but should I be resetting the search query back to '' whenever the flyout opens/closes? 🤔 Was it weird that the search input persisted...? Or was it cool?... maaybe??...
There was a problem hiding this comment.
I think as a user I typically expect state to reset whenever I close a modal, fwiw.
You know, as I started thinking about this, I saw the following and had a thought...
{isFlyoutOpen && <AddResultFlyout />}
I'm used to modals having their state reset automatically whenever they're closed because they are unmounted whenever isFlyoutOpen is false.
That's because the modal state is usually local to the modal component. I guess because the AddResultLogic state is higher in the component tree than AddResultFlyout, it never unmounts.
I don't have an opinion about whether or not this would be better or worse, but you could lift the isFlyoutOpen state up a level and push the logic down to be contained in AddResultFlyout.
That's just an open ended thought to consider.
There was a problem hiding this comment.
👍 to clearing state on modal open/close!
I guess because the AddResultLogic state is higher in the component tree than AddResultFlyout, it never unmounts
FWIW even if we removed {isFlyoutOpen && <AddResultFlyout />} from Curation.tsx, state would still be preserved on close. This is because we're calling const { flyoutOpen } = useActions(AddResultLogic) in PromotedDocuments & HiddenDocuments, and AddResultLogic being present in those views preserves the logic file and keeps it from unmounting. 🤷
EDIT: I think I misread your comment about changing where isFlyoutOpen is located, sorry 🤔 Not totally sure if that change is better or worse either personally. I think I like it where it is for unit testing - just IMO, it's slightly nicer to check conditionally for the presence of AddResultFlyout rather than checking for an empty render. And it prevents an extra level of indenting/nesting in AddResultFlyout, which is kinda nicer too. Not a big deal either way though like you said
There was a problem hiding this comment.
Commit that resets the search query: 6c559ac
JasonStoltz
approved these changes
Mar 18, 2021
|
|
||
| import { AddResultLogic } from './'; | ||
|
|
||
| describe('AddResultLogic', () => { |
There was a problem hiding this comment.
Good call putting this in it's own logic
| searchQuery: [ | ||
| '', | ||
| { | ||
| search: (_, { query }) => query, |
There was a problem hiding this comment.
I think as a user I typically expect state to reset whenever I close a modal, fwiw.
You know, as I started thinking about this, I saw the following and had a thought...
{isFlyoutOpen && <AddResultFlyout />}
I'm used to modals having their state reset automatically whenever they're closed because they are unmounted whenever isFlyoutOpen is false.
That's because the modal state is usually local to the modal component. I guess because the AddResultLogic state is higher in the component tree than AddResultFlyout, it never unmounts.
I don't have an opinion about whether or not this would be better or worse, but you could lift the isFlyoutOpen state up a level and push the logic down to be contained in AddResultFlyout.
That's just an open ended thought to consider.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Firefox XPack UI Functional Tests.x-pack/test/functional/apps/grok_debugger/grok_debugger·js.logstash grok debugger app "before all" hook in "grok debugger app"Standard OutStack TraceMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @JasonStoltz |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Mar 18, 2021
* Set up curation search server route - not really sure which API endpoint to use, hedging my bets * Set up AddResultLogic - fairly simple, mostly concerned with flyout behavior & search query - could likely be reused (or replaced with??) query tester logic in the future * Add main AddResultFlyout component - with custom isPromoted / isHidden logic & actions * Update AddResultButton to open flyout * Update Curation page to render the flyout * PR feedback: reset search query on flyout re-open
kibanamachine
added a commit
that referenced
this pull request
Mar 19, 2021
* Set up curation search server route - not really sure which API endpoint to use, hedging my bets * Set up AddResultLogic - fairly simple, mostly concerned with flyout behavior & search query - could likely be reused (or replaced with??) query tester logic in the future * Add main AddResultFlyout component - with custom isPromoted / isHidden logic & actions * Update AddResultButton to open flyout * Update Curation page to render the flyout * PR feedback: reset search query on flyout re-open Co-authored-by: Constance <[email protected]>
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
The final Curation feature PR! 🎉 This adds in the 'Add result manually' functionality. This was previously a separate page in the standalone UI and is moving to a flyout for improved UX.
Screencaps
QA
Checklist