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

Ctrl-click on results #517

Merged
merged 29 commits into from
Nov 5, 2018
Merged

Ctrl-click on results #517

merged 29 commits into from
Nov 5, 2018

Conversation

s-pace
Copy link

@s-pace s-pace commented Nov 1, 2018

PR that:

  • Fix the highlighting of lvl1 and lvl2
  • Enable command + click
  • Enable link preview
  • Enable multi input
  • Add a playground to use served assets

@algobot
Copy link

algobot commented Nov 1, 2018

Deploy preview ready

Built with commit fe530ca

https://deploy-preview-517--docsearch.netlify.com

@s-pace
Copy link
Author

s-pace commented Nov 1, 2018

Live demo of the new search https://docsearch-pr517.surge.sh/

@s-pace s-pace requested review from pixelastic and Haroenv November 1, 2018 21:58
.prettierrc Outdated Show resolved Hide resolved
scripts/playground.html Outdated Show resolved Hide resolved
scripts/playground.html Outdated Show resolved Hide resolved
scripts/serve Outdated Show resolved Hide resolved
@@ -64,7 +64,7 @@ class DocSearch {
this.apiKey = apiKey;
this.appId = appId;
this.indexName = indexName;
this.input = DocSearch.getInputFromSelector(inputSelector);
this.input = DocSearch.getInputsFromSelector(inputSelector);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can now accept several selectors and not just one, I would rename this.input to this.inputs

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will enhance it with the string selectors

src/lib/DocSearch.js Outdated Show resolved Hide resolved
src/lib/templates.js Outdated Show resolved Hide resolved
src/lib/templates.js Outdated Show resolved Hide resolved
@@ -67,7 +71,9 @@ const templates = {
`,
footer: `
<div class="${footerPrefix}">
Search by <a class="${footerPrefix}--logo" href="https://www.algolia.com/docsearch">Algolia</a>
<a class="${footerPrefix}--logo" aria-label="Search by algolia" href="https://www.algolia.com/docsearch">
<img src="${searchByAddress}" alt="Search by algolia" height="100%" width="100%">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using an external url for the image. I would rather have it either as an inline SVG in the JavaScript, or as a background image from the CSS. In any case, if we use an URL, we should use one that targets an origin we control (ie. something in the DocSearch repo, or DocSearch website, not on the main Algolia website).

I'm gonna ping @JonasBa on this: How do you suggest we include the "Powered by Algolia" logo in DocSearch? As an inline SVG, a background image or a hotlink?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh neat.

src/styles/main.scss Show resolved Hide resolved
@pixelastic
Copy link
Contributor

I removed the multi-input feature as it was not working as intended (we only return one instance, so we cannot bind it to two different inputs). I also remove the two inputs from the playground (keeping only one), as we don't yet handle multiple inputs, it was confusing.

I changed the handleSelected logic so it works with links. Basically by default a a is enough to redirect people to the destination on click (we still need our own handleSelected for keyboard navigation though). But when a specific handleSelected is defined by the user, it should take precedence. When such a handleSelected is defined, we prevent all links from executing their default behavior by catching all click on those elmeents and calling event.preventDefault().

This works well from my manual tests, I still have one unit test to write and it should be ok.

@pixelastic pixelastic changed the title Feat/v3 Ctrl-click on results Nov 5, 2018
Copy link
Author

@s-pace s-pace left a comment

Choose a reason for hiding this comment

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

Need test and we go live 🎉

.prettierrc Outdated Show resolved Hide resolved
@s-pace s-pace merged commit fe530ca into master Nov 5, 2018
@s-pace
Copy link
Author

s-pace commented Nov 5, 2018

@pixelastic I wanted to resolve automatically the conflicts. I have used the hint provided by github. It actually merges everything.
Feel free to open a PR to add test, then we can release it

@pixelastic pixelastic deleted the feat/v3 branch November 5, 2018 13:07
@pixelastic
Copy link
Contributor

Wait, we should not release this as v3, but keep it a minor bump.

If we release it as v3, people won't be able to use it right away, as it would require them to update the link to the CDN:
image

We need to make sure it's still a v2.x, with backward compatibility. We want to keep the big bump to v3 for the rewrite, where we'll be able to break backward compat if needed.

@s-pace
Copy link
Author

s-pace commented Nov 5, 2018

I think we do not have any issue with releasing it straight away. I do not see any breaking change. Use os really regular so let's release the v2 :)
(We will help people to solve issue if needed)

@s-pace
Copy link
Author

s-pace commented Nov 5, 2018

Could you have a look regarding gzip or any way to reduce the size of it pls?

@pixelastic
Copy link
Contributor

pixelastic commented Nov 5, 2018

I think we do not have any issue with releasing it straight away.

We're still missing at least one test. I would not release something that hasn't been tested

Could you have a look regarding gzip or any way to reduce the size of it pls?

Yes, and I think the logo display is bigger in that branch than it used to be. I'd like to double check that before the release as well.

@s-pace
Copy link
Author

s-pace commented Nov 5, 2018

Fine with it. Write a test and then we can release 🎉

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.

3 participants