neo4j-documentation / docs-ui

Mozilla Public License 2.0
4 stars 23 forks source link

fix kb search options #74

Closed spgandhi closed 3 years ago

spgandhi commented 3 years ago

This PR fixes a small typo to the /kb search options. @adam-cowley

netlify[bot] commented 3 years ago

✔️ Deploy Preview for neo4j-docs-ui ready!

🔨 Explore the source changes: 7ca3a0378d185f1b28f80b6ce2dd080492620926

🔍 Inspect the deploy log: https://app.netlify.com/sites/neo4j-docs-ui/deploys/613f654c9aa89c0007896a67

😎 Browse the preview: https://deploy-preview-74--neo4j-docs-ui.netlify.app

ggrossetie commented 3 years ago

It does not work for at least two reasons:

Are we using custom code for this logic? I can only see the minified code (https://neo4j.com/wp-content/themes/neo4jweb/assets/neo4j-react-modules-assets/search-preact/chunkless/bundle.js).

i.getSearchOptions = function(e) {
                var t = {};
                if (e) {
                    var n, r, i, a = e.target.getAttribute("search-index"), o = e.target.getAttribute("search-placeholder"), s = e.target.getAttribute("search-template"), c = null;
                    try {
                        var u = e.target.dataset.searchOptions.replace(/'/g, '"');
                        (c = JSON.parse(u)).defaultRefinement && "string" == typeof c.defaultRefinement && (c.defaultRefinement = [c.defaultRefinement])
                    } catch (e) {
                        console.warn("someething wrong in parsing search options")
                    }
                    t = {},
                    (a || null != (n = c) && n.indexName) && (t[a] = a || c.indexName),
                    (o || null != (r = c) && r.placeholder) && (t[o] = o || c.placeholder),
                    (s || null != (i = c) && i.template) && (t[s] = s || c.template)
                }
                return Re(Re(Re({}, jr), window.algoliaSearchOptions), t)
            }

t object

{null: "docs"}

c = JSON.parse(e.target.dataset.searchOptions)

{indexName: "docs", template: "docs", defaultRefinement: Array(1), placeholder: "Search Knowledge Base"}

window.algoliaSearchOptions

{indexName: "docs", placeholder: "Search Documentation", template: "docs"}

return Re(Re(Re({}, jr), window.algoliaSearchOptions), t)

{indexName: "docs", placeholder: "Search Documentation", template: "docs", defaultRefinement: Array(0), null: "docs"}
ggrossetie commented 3 years ago

You can debug/troubleshoot using https://deploy-preview-74--neo4j-docs-ui.netlify.app/kb-index.html

spgandhi commented 3 years ago

@Mogztter thanks for pointing that out. I just pushed a patch to the search module. By replacing e.target to e.currentTarget solves the problem you mentioned above. On the netlify URL, the search won't show results because the Dev env of algolia does not have docs related data. Although this PR has a very small change and should be good to go.

ggrossetie commented 3 years ago

Something is still not working:

image

As you can see the placeholder is "Search Documentation" instead of "Search Knowledge Base"

spgandhi commented 3 years ago

@Mogztter just pushed another fix and the placeholder is now working. Thanks for pointing that.

spgandhi commented 3 years ago

@adam-cowley just checking on this one if we can merge it

adam-cowley commented 3 years ago

I still see Search Documentation rather than Knowledge Base on the preview URL after clearing the cache and redeploying. Could you take a look please @spgandhi ?

https://deploy-preview-74--neo4j-docs-ui.netlify.app/kb-index.html

spgandhi commented 3 years ago

@adam-cowley I opened the URL in the incognito and am able to see it work correctly. @Mogztter what do you see on your end?

Screenshot 2021-07-30 at 12 18 37 PM

ggrossetie commented 3 years ago

@spgandhi It's working when I'm clicking on the search bar in the hero panel. However, it does not work when I'm using the shortcut "/" nor when I'm clicking on the magnifying glass icon in the top menu (right corner).

spgandhi commented 3 years ago

@Mogztter that behaviour desired. Only the KB search in the hero is supposed to have the custom placeholder "Search Knowledgebase" and have default filter of KB items. When you click on search in nav or press "/" you are invoking the global search which is entire Docs search

cc: @adam-cowley

ggrossetie commented 3 years ago

In this case, I think the design is a bit misleading:

image

For me, it seems that the shortcut "/" will only search in the Knowledge Base but let's hear what Adam has to say.

spgandhi commented 3 years ago

I agree, the hero search should not have the "/" in it. Should I make that part of this PR, do we want to tackle it separately? @adam-cowley @Mogztter

spgandhi commented 3 years ago

Hi @adam-cowley can we get this merged please?