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.

Sign up for GitHub

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

Jump to bottom

adjust compatibility with the new search protocol #328

Closed
wants to merge 0 commits into from
Closed

adjust compatibility with the new search protocol #328

wants to merge 0 commits into from

Conversation

hiagolcm
Copy link
Contributor

@hiagolcm hiagolcm commented Mar 19, 2020 •
edited
Loading

What problem is this solving?

Currently, VTEX has its own protocol between the search API and the UX components. This protocol forces new search-engines to implement specific rules to communicate with the UI. The search-protocol project purposes a generic way to make the UI/API communication.

This PR adds a "compatibility layer" to handle the new search-protocol. With this layer, there is no need to make further changes in the UI components.

It also adds three new props that are essential for new search engines. They are: fuzzy, operator and searchState. These props are in the URL as queryStrings.

Finally, this PR makes a slight improvement in the FilterNavigator by avoiding unnecessary mounting.

How should this be manually tested?

I have created two workspaces, each one with a search engine

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

Notes

This PR depends on

@hiagolcm hiagolcm requested a review from a team as a code owner March 19, 2020 12:42
@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Mar 19, 2020

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch

  • Minor

  • Major

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@vtex-io-docs-bot
Copy link

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

jgfidelis
jgfidelis reviewed Mar 19, 2020
View reviewed changes
react/components/SearchQuery.js Outdated
const pageRef = useRef(page)
const isCurrentDifferent = (ref, currentVal) => ref.current !== currentVal

const useShouldResetPage = (query, map, orderBy) => {
const queryRef = useRef(query)
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 this might be a moment to refactor all of these refs to use a reducer state model.

Then you can just dispatch the reset action when needed. It may be done in a later PR though.

Copy link
Contributor Author

@hiagolcm hiagolcm Mar 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nice suggestion! I could make this PR by myself when I have a chance.
I have just created an issue about that.
#330

jgfidelis
jgfidelis reviewed Mar 19, 2020
View reviewed changes
react/hooks/useFacetNavigation.js Outdated
map: `${currentMap}`,
query: `/${currentQuery}`,
page: undefined,
})
}
if (fuzzy) queries.fuzzy = fuzzy
Copy link
Contributor

Choose a reason for hiding this comment

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

better to keep pattern and use if (x) { fn() }

jgfidelis
jgfidelis reviewed Mar 19, 2020
View reviewed changes
react/hooks/useFetchMore.js Outdated
setQuery({ page: pageState.previousPage }, { replace: true, merge: true })

const queries = { page: pageState.previousPage }
if (fuzzy) queries.fuzzy = fuzzy
Copy link
Contributor

Choose a reason for hiding this comment

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

you should abstract this logic in a function, its used like 5 times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realized that I can accomplish the same result by using fuzzy: fuzzy || fuzzy.
I think it is better now.

kaisermann
kaisermann reviewed Mar 19, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
react/FilterNavigator.js Outdated
})

if (loading && !mobileLayout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move separate ifs to nested ternaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, this statement is inside a Fragment, and I can't use the if inside the curly braces.
Do you think I should create a new function to handle this JSX? This way, I would be able to use the if again.

react/FilterNavigator.js Outdated Show resolved Hide resolved
iaronaraujo
iaronaraujo suggested changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@iaronaraujo iaronaraujo left a comment

Choose a reason for hiding this comment

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

Is this linked in this workspace? If so, non-ft pages seem to be broken. Also, have you rebased this app? There may be some conflicts with the new URL change.

@hiagolcm
Copy link
Contributor Author

Is this linked in this workspace? If so, non-ft pages seem to be broken. Also, have you rebased this app? There may be some conflicts with the new URL change.

@iaronaraujo I forgot to mention that vtex-search can't solve non-ft pages yet 🤦‍♂ . So broken non-ft pages are totally expected. I only added this workspace to show that now it is possible to set a different search engine.

I have rebased the app today, the last release was the v3.51.4. It looks ok to me!

iaronaraujo
iaronaraujo approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@iaronaraujo iaronaraujo left a comment

Choose a reason for hiding this comment

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

Code LGTM, but if possible link this with our normal search and check if the page URL parameter is still working as intended, please!

@hiagolcm hiagolcm force-pushed the feature/adjust-compatibility-with-the-new-search-protocol branch 2 times, most recently from b9c462b to ad4dc02 Compare April 7, 2020 12:38
@hiagolcm hiagolcm requested a review from a team as a code owner April 7, 2020 12:38
@hiagolcm hiagolcm force-pushed the feature/adjust-compatibility-with-the-new-search-protocol branch from ad4dc02 to b0661ec Compare April 7, 2020 12:57
@hiagolcm hiagolcm closed this Apr 7, 2020
@hiagolcm hiagolcm force-pushed the feature/adjust-compatibility-with-the-new-search-protocol branch from b0661ec to 7cc0e93 Compare April 7, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iaronaraujo iaronaraujo iaronaraujo approved these changes

@klzns klzns Awaiting requested review from klzns

@Jeymisson Jeymisson Awaiting requested review from Jeymisson

@jgfidelis jgfidelis Awaiting requested review from jgfidelis

@kaisermann kaisermann Awaiting requested review from kaisermann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hiagolcm @jgfidelis @iaronaraujo @kaisermann