Skip to content
This repository has been archived by the owner on Jan 20, 2023. It is now read-only.

GROVE-334 : adds 'show more' link to show more facets in the Vue template #8

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

janmichaelyu
Copy link

No description provided.

@withjam withjam requested review from withjam and dalaidunc August 8, 2019 14:46
@mariannemyers mariannemyers requested a review from grtjn August 9, 2019 21:44
@mariannemyers
Copy link

This looks good to @withjam, @dalaidunc, and me. Waiting for final review from @grtjn when he's back from holiday before it is merged.

@grtjn
Copy link
Contributor

grtjn commented Aug 13, 2019

Again, based off of master, and it looks like similar-query changes are mixed in too. Makes review a little harder.

I think we would eventually want to move some of this logic into middle-tier, but it is an improvement over what we have now. Let me try to run a quick sanity check, just to make sure all is complete, and i'll merge after that.


getValues(name, params, options, searchState, qtext, facetObject) {
//get options
return fetch('/v1/config/query/all?format=json', {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should make a call to /search/{type}/config.. (see comments in grove-node PR)

text: path,
ns: ''
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to just copy the entire range object from the constraint. Might not be string, might not be path index..

ns: ''
}
},
'values-option' : ['frequency-order']
Copy link
Contributor

Choose a reason for hiding this comment

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

copy these from facet-options.. some facets use item-order for sure..

'values-option' : ['frequency-order']
};
//combine with search
let searchType = 'all';
Copy link
Contributor

Choose a reason for hiding this comment

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

searchType should be provided as param, like in searchApi..

});

return temp;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think latest searchApi in development branch has captured this in a helper function that you could expose and reuse here..

// let searchType = searchType !== undefined ? searchType : 'all';
let start = facetObject.facetValues.length +1 || 1;
// let start = 1;
let pageLength = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using facet limit as page-length?

valuesParams.append('start', start);
valuesParams.append('pageLength', pageLength);
valuesParams.append('limit', limit);
valuesParams.append('direction', 'descending');
Copy link
Contributor

Choose a reason for hiding this comment

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

direction should match direction of facet..

valuesParams.append('direction', 'descending');
let valuesParamsString = valuesParams.toString();

return fetch('/v1/values/' + name + '?'+valuesParamsString, {
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to getting of config, make a call to /search/{type}/values instead. Consider passing above params through via post body rather than request params, and rewrite them in middle-tier to request params, though perhaps something for bonus..

showMore(facet, facetName) {
if (facet.displayingAll) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be unnecessary I think. the button should simply not show if displayingAll is true..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants