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

PL-6 Generalize the Search API fields #39

Merged
merged 13 commits into from
Sep 19, 2018

Conversation

Kbentham
Copy link
Contributor

@Kbentham Kbentham commented Aug 6, 2018

https://palantir.atlassian.net/browse/PL-6

This module splits out the Search API Fields into a new module search_api_field_map. Can be tested using the demo site we have set up. See PL-6-generalize-search-api-fields for more information.

Copy link
Contributor

@froboy froboy left a comment

Choose a reason for hiding this comment

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

As suggested on the ticket, this should include two modules: Search API Mapped Field (search_api_mapped_field) and Search API Mapped Terms (search_api_mapped_terms). These will eventually go into a new repo entitled Search API Mapped (search_api_mapped) (not sure about this name). The new module should include a suggestion that it's recommended to use in tandem with search_api_federated_solr.

@Kbentham please move the submodule to the new repo, https://github.com/palantirnet/search_api_field_map and ignore the above about splitting fields and terms. Just keep it all together.

@@ -0,0 +1,13 @@
langcode: en
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't directly related to the fields implementation so it should stay in search_api_federated_solr.

@froboy
Copy link
Contributor

froboy commented Aug 27, 2018

I think https://github.com/palantirnet/search_api_field_map should be added as a dependency to this module.

@@ -4,13 +4,13 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the same I have removed the file.

@froboy
Copy link
Contributor

froboy commented Aug 29, 2018

@Kbentham in testing I noticed that some of the fields (title, image) aren't getting rendered on the page here. It looks like https://github.com/palantirnet/search_api_federated_solr/blob/master/search_api_federated_solr.module#L76 needs to be updated with the new field names. That should... i think... still stay in this module though, as it's necessary for the react things, but not necessarily other uses cases.

Current data in solr (on this PR):

"sm_federated_image": [
  "http://federated-search-demo.local/sites/default/files/styles/search_api_federated_solr_image/public/mushrooms-umami.jpg?itok=2-j6RNl0"
],
"sm_federated_title": [
  "The umami guide to our favorite mushrooms"
],

Desired data (from #40)

"ss_federated_image": "http://federated-search-demo.local/sites/default/files/styles/search_api_federated_solr_image/public/mushrooms-umami.jpg?itok=YKyunezh",
"ss_federated_title": "The umami guide to our favorite mushrooms",

@Kbentham
Copy link
Contributor Author

Kbentham commented Sep 4, 2018

@froboy I fixed the issues you found.

@froboy
Copy link
Contributor

froboy commented Sep 19, 2018

@Kbentham I generalized that statement, and it should work for other fields if they exist now. Thanks for your work, this looks good now.

@froboy froboy merged commit a04466b into master Sep 19, 2018
@froboy froboy deleted the PL-6-generalize-search-api-fields branch September 19, 2018 19:18
jesconstantine added a commit that referenced this pull request May 29, 2019
add #39 to 7.x-2.x umhs-28: use canonical URL for results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants