Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: query preview components allow adding extra params to the request #1270

Merged

Conversation

CachedaCodes
Copy link
Contributor

@CachedaCodes CachedaCodes commented Aug 29, 2023

EMP-1555

Motivation and context

We need to gain more control with the parameters that we use to create the query preview requests. Right now we were limited to the query. In this PR we add the feature of making the query preview request with extra params too

  • Dependencies. If any, specify:
  • Open issue. If applicable, link:

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that causes existing functionality to not work as expected)
  • Change requires a documentation update

What is the destination branch of this PR?

  • Main
  • Other. Specify:

How has this been tested?

Select a query preview that has been initialized with custom extra params. Check that the search uses those extra params until it is restarted. Check that the search uses the initial extra params afterwards.

annacv and others added 30 commits July 31, 2023 13:14
@annacv
Copy link
Contributor

annacv commented Aug 29, 2023

Hi, selecting a query preview and then deleting it and selecting another queryPreview with different params is not working properly, params that change between requests are not kept (instance, store in the examples) no matter if the changes proposed are applied or not:

Gravacio.de.pantalla.2023-08-29.a.les.16.00.41.mov

@CachedaCodes CachedaCodes marked this pull request as ready for review September 4, 2023 13:01
@CachedaCodes CachedaCodes requested a review from a team as a code owner September 4, 2023 13:01
@@ -197,7 +198,11 @@
protected created(): void {
this.$watch(
() => this.queryPreviewRequest,
request => this.emitQueryPreviewRequestUpdated(request)
(newRequest, oldRequest) => {
if (JSON.stringify(newRequest) !== JSON.stringify(oldRequest)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could use getNewAndUpdatedKeys for this?

packages/x-components/src/views/home/Home.vue Show resolved Hide resolved
@@ -348,8 +353,8 @@
</h1>
<LocationProvider :location="$x.noResults ? 'no_results' : 'low_results'">
<QueryPreviewList
:queries="queries"
#default="{ query, results }"
:queries-preview-info="queries.map(q => ({ query: q }))"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this to a property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but queries comes from the scope slot from SemanticQueries, I can't make it a property. I could move the mapping to a function that receives the queries list.

*
* @internal
*/
protected get events(): Partial<XEventsTypes> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if it would make sense to add a prop to allow emit other events that are configured from the outside. I think that approach came handy in the past in some setups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, this component is not much more than a BaseEventButton, don't know if it's worth it. Will think about it anyways

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!
That's a good point to have in mind, but I think we should go on to be able to implement the filters part ASAP.

diegopf
diegopf previously approved these changes Sep 12, 2023
…y-preview-with-filters-and-extraparams

# Conflicts:
#	packages/x-components/src/x-modules/queries-preview/components/query-preview.vue
@CachedaCodes CachedaCodes requested a review from a team as a code owner September 12, 2023 10:53
Comment on lines 28 to 32
/**
* The selected query preview has changed.
* Payload: The new selected query preview with extra params or the initial module
* params if the query has been cleared.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!
If I am not wrong, I think this doc should be updated to be more precise, at least the payload description, as now it is not a change event that will always be fired.

Copy link
Contributor

@annacv annacv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@CachedaCodes CachedaCodes merged commit 84143a2 into main Sep 15, 2023
4 checks passed
@CachedaCodes CachedaCodes deleted the feature/EMP-1554-Query-preview-with-filters-and-extraparams branch September 15, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants