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

global: react-searchkit integration #76

Merged

lnielsen merged 12 commits into inveniosoftware:master from kpsherva:searchkit-integration

May 15, 2020

Contributor

kpsherva commented May 8, 2020 •

edited

Loading

choose js framework
closes Integrate React-SearchKit #75

kpsherva force-pushed the searchkit-integration branch from af2666b to a7be4eb Compare

May 8, 2020 14:25

kpsherva self-assigned this

kpsherva changed the title ~~global: react-searchkit integration~~ [BLOCKED] global: react-searchkit integration

ntarocco reviewed

View reviewed changes

invenio_search_ui/assets/js/invenio_react_search_ui/App.js Outdated

+              import { Results } from "./Results";
+              import { InvenioSearchApi } from "react-searchkit";
+              import "semantic-ui-css/semantic.min.css";

Contributor

ntarocco May 12, 2020

note: this will have to be removed when integrated in the global Invenio theme

Contributor Author

kpsherva May 12, 2020

yes, I'm waiting for the theme to be available to remove that

invenio_search_ui/assets/js/invenio_react_search_ui/App.js Outdated


		render() {

		const config = new InvenioSearchApi({

Contributor

ntarocco May 12, 2020 •

edited

Loading

I would say that this should be configurable too. What about if you have:

const searchApi = this.props.searchApi || new InvenioSearchApi(...)

this should be possible for all the params that can be passed to the RSK, for at least most of them, see [https://inveniosoftware.github.io/react-searchkit/docs/components/react-searchkit] (I am not sure that the doc there is up-to-date).
How to inject that JS inside... something to be brainstormed! :)

Contributor Author

kpsherva May 12, 2020

👍

invenio_search_ui/assets/js/invenio_react_search_ui/App.js Outdated

+                          <Grid.Row columns={2}>
+                            <Grid.Column width={4}>
+                              <Grid.Row><Grid.Column/></Grid.Row>
+                              <BucketAggregation

Contributor

ntarocco May 12, 2020

We should use RECORDS_REST_FACETS to render these buckets

Contributor Author

kpsherva May 12, 2020

done, thanks for noticing this

invenio_search_ui/assets/js/invenio_react_search_ui/App.js Outdated


		const OnResults = withState(Results);

		const resultsPerPageValues = [

Contributor

ntarocco May 12, 2020

This should be also configurable, depending on the default that the user sets in RECORDS_REST_DEFAULT_RESULTS_SIZE

Contributor Author

kpsherva May 12, 2020

I removed this component in the end, as the variable allows only one option

invenio_search_ui/assets/js/invenio_react_search_ui/Results.js Outdated Show resolved Hide resolved

invenio_search_ui/assets/js/invenio_react_search_ui/Results.js Outdated

+                      <Item.Content>
+                        <Item.Header>{metadata.title}</Item.Header>
+                        <Item.Description>
+                          {_truncate(metadata.keywords.join(","), { length: 200 })}

Contributor

ntarocco May 12, 2020

@lnielsen in invenio-search-ui we had a marc21 folder with some fields displayed. What would be a sensible default here to display records? I guess it is for sure overridden in cookiecutter-invenio-instance, but what should we have here?

Member

lnielsen May 12, 2020

Everything related to marc21 can be removed.

Something like record.title + record.description

invenio_search_ui/assets/js/invenio_react_search_ui/Results.js Outdated

+                      <Grid relaxed>
+                        <Grid.Row>
+                          <Grid.Column width={16}>
+                            <ActiveFilters />

Contributor

ntarocco May 12, 2020

IMHO, I would remove this one in the default invenio-search-ui

Contributor Author

kpsherva May 12, 2020

removed.

ntarocco reviewed

View reviewed changes

invenio_search_ui/assets/js/invenio_react_search_ui/Results.js Outdated

+                }
+              }
+              Results.propTypes = {};

Contributor

ntarocco May 12, 2020

Please make sure that at some point (probably later?) you fill in all propTypes and defaultProps :)

kpsherva force-pushed the searchkit-integration branch 3 times, most recently from 31e4b1c to a502595 Compare

May 12, 2020 16:03

ntarocco reviewed

View reviewed changes

invenio_search_ui/assets/js/invenio_react_search_ui/Results.js Outdated

+                renderResultsListItem = (result, index) => {
+                  const metadata = result.metadata;
+                  return (
+                    <Item key={index} href={_get(result, "links.html", "#")}>

Contributor

ntarocco May 12, 2020

I solved the link like this: inveniosoftware/cookiecutter-invenio-instance#226
(also removed truncate and change style... do we actually need lodash)?

Contributor Author

kpsherva May 13, 2020

I don't see the solution for the links.html under your link. Could you explain in more details, which line do you mean?

ntarocco reviewed

View reviewed changes

invenio_search_ui/assets/js/invenio_react_search_ui/Results.js Outdated

+                      <Grid.Row verticalAlign="middle">
+                        <Grid.Column width={7}>
+                          <Count
+                            renderElement={this.renderCount}

Contributor

ntarocco May 12, 2020

be aware render element has disappeared in the next version of RSK

Contributor Author

kpsherva May 13, 2020

I remember, actually I could just remove it

kpsherva force-pushed the searchkit-integration branch from ca96b7e to 981323b Compare

May 13, 2020 13:01

lnielsen reviewed

View reviewed changes

setup.py Outdated Show resolved Hide resolved

invenio_search_ui/webpack.py Outdated

+                  themes={
+                      'semantic-ui': dict(
+                          entry={
+                              'search_ui_app': './js/invenio_react_search_ui/index.js',

Member

lnielsen May 13, 2020

Suggested change

      
                            'search_ui_app': './js/invenio_react_search_ui/index.js',
          
                            'search_ui_app': './js/invenio_search_ui/react/index.js',

I would do the namespacing like suggested (<module name>/react/) or simply just use without react part.

Ditto for below on the angular app. -> invenio_search_ui/angular/app.js

Contributor Author

kpsherva May 13, 2020

fixed

invenio_search_ui/views.py Outdated

+                  config_default_sort = config.get('RECORDS_REST_DEFAULT_SORT', {}).get(
+                      search_index)
+                  return json.dumps({

Member

lnielsen May 13, 2020

Minor: Use the Jinja filter |tojson instead. It's not a big issue since there's no user-provided data, but tojson does proper escaping to prevent XSS attacks, so that the JSON can safely be included in HTML script tags.

I can see this is an inherited problem from the module :-)

Contributor Author

kpsherva May 13, 2020

fixed

invenio_search_ui/config.py Outdated

		@@ -10,6 +10,8 @@

		from __future__ import absolute_import, print_function

		APP_THEME = ['semantic-ui', 'bootstrap3']

Member

lnielsen May 13, 2020

Comment: I guess this is also just for testing?

Contributor Author

kpsherva May 13, 2020 •

edited

Loading

I left it until I'm sure where this should go, cookiecutter right? Also I'm still not 100% sure if the js framework should be "hidden" under themes= names

kpsherva force-pushed the searchkit-integration branch 3 times, most recently from 3590535 to a5aca8f Compare

May 13, 2020 14:08

kpsherva added 4 commits

May 13, 2020 18:37


          global: react-searchkit integration

ba31426

* choose js framework
* layout fixes


          react-searchkit: theme semantic-ui integration

44e0870


          react-searchkit: overridable integration

a6c5f2a


          react-searchkit: portals integration

9e332c7

* ID-assigned searchbar
* remove double searchbar

kpsherva force-pushed the searchkit-integration branch from 6f0253d to 9e332c7 Compare

May 13, 2020 16:38

Contributor

ntarocco commented May 13, 2020

There are issues when integrating with cookiecutter with the id of the header and React portals, we will need to fix it tomorrow.


          global: bump invenio packages

bde075f

ntarocco linked an issue

that may be closed by this pull request

Add search bar outside the react-searchkit app #78

Closed

ntarocco changed the title ~~[BLOCKED] global: react-searchkit integration~~ global: react-searchkit integration

lnielsen approved these changes

View reviewed changes

lnielsen assigned lnielsen and unassigned kpsherva

ntarocco and others added 5 commits

May 15, 2020 07:53


          installation: change dependencies

786d256


          react-searchkit: fix searchbar attachement

f4ab3a3


          global: remove assets and overall fixes

880f91a


          react-searchkit: fix layouting


          tests: fix tests

08a6cc6

lnielsen force-pushed the searchkit-integration branch from b210ebd to 9bb42a7 Compare

May 15, 2020 05:57

lnielsen added 2 commits

May 15, 2020 07:58


          installation: remove anuglar-gettext

8267b73


          release: v2.0.0a1

2fecd7a

lnielsen force-pushed the searchkit-integration branch from 9bb42a7 to 2fecd7a Compare

May 15, 2020 05:58

lnielsen merged commit 2fecd7a into inveniosoftware:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet