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 #9

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

Conversation

janmichaelyu
Copy link

No description provided.

@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

Mike, you worked against master, rather than development. That probably explains why it is showing some commits from Marianne right now. It makes it easier to review if the PR only shows the relevant commits. Not a big deal though, and it doesn't seem to cause trouble for merging..

package.json Outdated
@@ -25,7 +34,6 @@
"start": "cross-env NODE_ENV=development nodemon node-app.js",
"start:prod": "node node-app.js"
},
"private": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

@mariannemyers it looks like you dropped the private flag, and published grove-node on npm, but it was marked as private on purpose. grove-node is part of the grove templates, not a package.. Let's discuss in the next meeting what we do with this..

authed: true
},
{
endpoint: '/config/query/all', // NOTE: allows get on all extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Allows getting the 'all' search options..

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 would be nice if we would expose proper rest endpoints for both of these. so, maybe /search/all/config for the latter, and /search/all/values for the first. It might be possible to leverage this work to rewrite those calls to other urls against ML: #29

From top of my head, something like:

route.use('/search/' + type, routeFactory.defaultExtensionRoute({
    authProvider,
    actions: {
        config: {
            uri: '/v1/config/query/all',
            GET: function(body, params, req){
                return {
                    method: req.method,
                    body: '',
                    params: {}
                }
            }
        }
    }

It may be necessary to patch either the default search or default ext route to fall through, otherwise one or the other might not get reached..

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you have a play with that @janmichaelyu ?

}
})
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for taking so long, but after rethinking about this, I am thinking we should not need a separate /search/similar endpoint. Instead, similar should just be 'yet another constraint' in the existing all.xml. Could you revisit this PR (you will have to open a new one unfortunately), and move that custom constraint into all.xml?

https://github.com/marklogic-community/grove-ml-gradle/pull/3/files

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