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(hits): introduce hits fullpath in search results #183

Merged

Conversation

jerrytohvan
Copy link
Contributor

Creating PR for proposal added here: #182 (comment)

@jerrytohvan
Copy link
Contributor Author

Head's up that I need help and guidance to check if this is the best implementation. I was also encountering blockers from running local environment to test this properly.

@Jerska
Copy link
Member

Jerska commented Nov 6, 2023

This looks like a great addtion! Can you share which blockers you faced on the local setup?

@jerrytohvan
Copy link
Contributor Author

jerrytohvan commented Nov 7, 2023

image

@Jerska , i received the above error when running npm start and didnt manage to get clean npm install. Running node on version 10.24.1 currently. Anything that I might have missed?

@jerrytohvan
Copy link
Contributor Author

Hi @Jerska , following up on this if you are able to assist with pull request? :)

@Jerska
Copy link
Member

Jerska commented Nov 20, 2023

Sorry for not getting back to you earlier!

I just merged a PR that should make it easier to run locally, I'd recommend you to pull master again & retry npm install && npm run dev (sorry this repo is quite old, and libs that were cutting edge back them are so old today...).

That being said, I can confirm it works:
image

It's a bit heavy in terms of effects for the general library, I have a few changes to recommend that would make them look like so:
image

Copy link
Member

@Jerska Jerska left a comment

Choose a reason for hiding this comment

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

Except for a few cosmetic changes, this looks great to me, thank you for the Pull Request!
@sbellone WDYT?

app/css/index.scss Outdated Show resolved Hide resolved
app/css/index.scss Outdated Show resolved Hide resolved
app/src/templates.js Outdated Show resolved Hide resolved
jerrytohvan and others added 3 commits November 21, 2023 11:28
Co-authored-by: Matthieu Dumont <[email protected]>
Co-authored-by: Matthieu Dumont <[email protected]>
Co-authored-by: Matthieu Dumont <[email protected]>
@jerrytohvan
Copy link
Contributor Author

Thanks for the review and I've pushed your suggestions @Jerska !

Copy link
Contributor

@sbellone sbellone left a comment

Choose a reason for hiding this comment

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

Nice addition indeed!

@sbellone sbellone merged commit 06bd790 into algolia:master Nov 21, 2023
@jerrytohvan
Copy link
Contributor Author

jerrytohvan commented Nov 22, 2023

Thank you team @sbellone @Jerska ! 👏
Are there timeline on next package version being published for usage? :)

@jerrytohvan jerrytohvan deleted the feat/introduce-full-path-to-search-results branch November 22, 2023 03:59
@Jerska
Copy link
Member

Jerska commented Nov 22, 2023

I'll release. :)

@Jerska
Copy link
Member

Jerska commented Nov 22, 2023

Had to fight the dependencies of all systems, but it's freshly released in 2.32.0!
See:

@jerrytohvan
Copy link
Contributor Author

thank you all!

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