backoffice: items and loans search #66

ntarocco · 2018-11-16T14:05:28Z

closes Back-office ui: implement mockups for search/list pages #58

topless · 2018-11-20T14:31:32Z

invenio_app_ils/ui/backoffice/src/common/api/base.js

+
+export const http = axios.create(apiConfig);
+
+const toIsoString = date => {


I would suggest to avoid custom date parsing, and go on with the de facto momentjs.

Well, I have evaluated that already, but it is a NOGO for what we need:

https://bundlephobia.com/result?p=moment@2.22.2

NPM package size of moment.js is bothersome moment/moment#3376

Actually, in my current develpoment (next PR) I am finding my self manipulating dates again, so it makes sense now to add a lib since there is nothing native that works nicely.
After investigation, it looks like Luxon (Memento child) is a good compromise. Here some "why".
I am using an it looks quite nice. WDYT? (ping @zzacharo @kprzerwa)

👍 Luxon looks quite good to me for what they describe in their docs

date-fns could be a candidate, I haven't use it though so no strong opinions, here is a comparison with moment.

topless · 2018-11-20T14:40:43Z

invenio_app_ils/ui/backoffice/src/common/components/Error/Error.js

+    let message;
+    switch (status) {
+      case 401:
+        message = 'You are not authenticated. Please login first.';


Does the error response from server come with a message? If so, would it make sense to use what comes out of the box?

Well, the default message is not very user friendly. For 401:
"message": "The server could not verify that you are authorized to access the URL requested. You either supplied the wrong credentials (e.g. a bad password), or your browser doesn't understand how to supply the credentials required.",

Imaging that reading our message librarians should understand what to do.

It doesn't come out of the box, it comes with the box ...

topless · 2018-11-20T15:07:43Z

invenio_app_ils/ui/backoffice/src/pages/ItemDetails/state/types.js

-export const IS_LOADING = 'IS_LOADING';
-export const ITEM_DETAILS = 'ITEM_DETAILS';
-export const ITEM_DETAILS_HAS_ERROR = 'ITEM_DETAILS_HAS_ERROR';
+export const IS_LOADING = 'item details/IS_LOADING';


Prefixing the actions with the actual caller component looks clean and intuitive.
About the prefix though I think I would go for ItemDetails as its class or any other pre-agreed combination of camel or snake case, but the space there is killing me.

I knew it! I actually commented with @zzacharo saying: "I am sure that if I put a space Chris won't be happy!" but I played my chances :)
I can change it if preferred. (@kprzerwa)

Example:

I also think we should change the space 😉

topless · 2018-11-20T15:17:18Z

invenio_app_ils/ui/backoffice/src/pages/ItemsSearch/__tests__/SearchBar.js

@@ -0,0 +1,74 @@
+import React from 'react';


I might be terribly wrong, but these look like tests for the SearchBar input. I am under the impression that react-search-kit comes with one out of the box. The point I am trying to convey here is that it should not part of the ils application to test each library's functionality, if that happens to be the case.

Well, I am overriding template and behaviour. React-SearchKit will come with tests for default implementations, but if you override and provide your own...

invenio_app_ils/ui/backoffice/src/pages/LoanDetails/state/types.js

kpsherva · 2018-11-21T08:47:40Z

invenio_app_ils/ui/backoffice/src/common/api/base.js

+
+export const http = axios.create(apiConfig);
+
+const toIsoString = date => {


👍 Luxon looks quite good to me for what they describe in their docs

kpsherva · 2018-11-21T08:53:02Z

invenio_app_ils/ui/backoffice/src/common/api/item.js

 };

-const getRecord = itemId => {
-  return http.get(`${itemURL}${itemId}`);
+const mapStatus = s => {


do you think it would make sense to define those human readable states in the backend and get them from there?

IMHO, REST API should never change data, because you don't know which client is going to consume it.
It is exactly the responsibility of the client to present data in human readable format, depending on the context (users, timezone, device, etc...)

removed mappings

kpsherva · 2018-11-21T09:04:18Z

invenio_app_ils/ui/backoffice/src/pages/ItemDetails/state/types.js

-export const IS_LOADING = 'IS_LOADING';
-export const ITEM_DETAILS = 'ITEM_DETAILS';
-export const ITEM_DETAILS_HAS_ERROR = 'ITEM_DETAILS_HAS_ERROR';
+export const IS_LOADING = 'item details/IS_LOADING';


I also think we should change the space 😉

kpsherva · 2018-11-21T09:08:35Z

invenio_app_ils/ui/backoffice/src/pages/ItemsSearch/ItemsSearch.js

+} from './components';
+import './ItemsSearch.scss';
+
+const resultsPerPageValues = [


I think at some point we could have a separate config file for this kind of things, I have a feeling it will appear in more places, and it would be more comfortable for us to keep it in one place

This is going to be replaced by injecting Invenio config, without hardcoding values. It this particular case, there is no invenio config :) So yes, we should put in common somewhere eventually...
but what about if each search page has different value?

We could decide if it is better to centralize everything, and remember then which config is changing what, or to leave specific view config in its view, so it is easier to see what it will affect.

As discussed, you would like to fix this in your next PR

kpsherva · 2018-11-21T09:14:39Z

invenio_app_ils/ui/backoffice/src/pages/ItemsSearch/ItemsSearch.js

+    );
+  };
+
+  _renderEmptyResults = (queryString, resetQuery) => {


is it possible to make it reusable for other searches? Because I can see that some bits are repeated in the LoansSearch

Yep, but as discussed in the DevOps, it is a deliberate choice, I don't know how this thing will evolve and the look and feel is very undone now...
I have the feeling we will pass through the full UI when making it more sexy, and the we can do this refactorings.

As discussed, we will refactor this in the future

* closes inveniosoftware#58

ntarocco added the review label Nov 16, 2018

ntarocco force-pushed the loans-search branch 7 times, most recently from d31c573 to b397457 Compare November 20, 2018 15:02

topless reviewed Nov 20, 2018

View reviewed changes

topless requested a review from zzacharo November 20, 2018 16:21

topless assigned ntarocco Nov 20, 2018

topless requested a review from kpsherva November 20, 2018 16:33

ntarocco force-pushed the loans-search branch 2 times, most recently from 47a3735 to cb0a51f Compare November 20, 2018 18:03

kpsherva reviewed Nov 21, 2018

View reviewed changes

backoffice: items and loans search

7b0d531

* closes inveniosoftware#58

ntarocco force-pushed the loans-search branch from cb0a51f to 7b0d531 Compare November 21, 2018 14:00

zzacharo approved these changes Nov 21, 2018

View reviewed changes

kpsherva approved these changes Nov 21, 2018

View reviewed changes

ntarocco merged commit aea08de into inveniosoftware:master Nov 21, 2018

ntarocco removed the review label Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backoffice: items and loans search #66

backoffice: items and loans search #66

ntarocco commented Nov 16, 2018

topless Nov 20, 2018

ntarocco Nov 20, 2018

ntarocco Nov 20, 2018 •

edited

kpsherva Nov 21, 2018

topless Nov 21, 2018

topless Nov 20, 2018

ntarocco Nov 20, 2018

topless Nov 20, 2018

topless Nov 20, 2018

ntarocco Nov 20, 2018 •

edited

kpsherva Nov 21, 2018

ntarocco Nov 21, 2018

topless Nov 20, 2018

ntarocco Nov 20, 2018 •

edited

kpsherva Nov 21, 2018

kpsherva Nov 21, 2018

ntarocco Nov 21, 2018

ntarocco Nov 21, 2018

kpsherva Nov 21, 2018

kpsherva Nov 21, 2018

ntarocco Nov 21, 2018 •

edited

ntarocco Nov 21, 2018

kpsherva Nov 21, 2018

ntarocco Nov 21, 2018

ntarocco Nov 21, 2018


		export const http = axios.create(apiConfig);

		const toIsoString = date => {

backoffice: items and loans search #66

backoffice: items and loans search #66

Conversation

ntarocco commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntarocco Nov 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntarocco Nov 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntarocco Nov 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntarocco Nov 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntarocco Nov 20, 2018 •

edited

ntarocco Nov 20, 2018 •

edited

ntarocco Nov 20, 2018 •

edited

ntarocco Nov 21, 2018 •

edited