Bug 30160 - Rewrite cities admin view in React
Summary: Rewrite cities admin view in React
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-23 09:12 UTC by Jonathan Druart
Modified: 2023-06-17 15:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30160: Cities view rewrite using React (35.79 KB, patch)
2022-02-23 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30160: Don't use classes but hooks (31.62 KB, patch)
2022-02-28 16:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-02-23 09:12:05 UTC
On bug 14974 we tried to provide a solution to use the REST API routes for all CRUD operations. The patches got dropped and we implemented the DT wrapper for the list view on bug 25279.

We were expecting to see different implementations in React or Vue, but none came.

Here is a try in React, for discussion.
Comment 1 Jonathan Druart 2022-02-23 09:25:00 UTC
Created attachment 131031 [details] [review]
Bug 30160: Cities view rewrite using React

This patch, for discussion, is trying to provide the easier
implementation of the cities CRUD operation using React.

The react-bootstrap library is used to take profit of the different
bootstrap elements.

Note that it's for me an initiation at React and I don't have much skills.
I've tried to provide the more concise and readable approach, with as
less bugs as possible. Feedback welcome, especially if you know React
better than me!
The bad thing with this approach is that we are loosing the history,
clicking browser's back button won't work as before. I don't think we
can use the react-router approach as we are not having a full React app.

TODO:
* There are some UI glitches on the add/edit and delete form (TODO)
* Translation strings?

Test plan:
% yarn install
Hit Home > Administration > Cities
Add, edit and delete cities

Note that if you want to modify the JS code here you will need to run
% gulp build_js
in order to regenerate the built version.
Comment 2 Tomás Cohen Arazi 2022-02-23 11:24:33 UTC
Are you willing to try Vue as well? I fear index.js will grow too much when adding more components. I'm not on either camp but I remember Vue was better suited for modernizing legacy projects that don't intend to get converted into a full-fledged SPA.

Great work, by the way.
Comment 3 Jonathan Druart 2022-02-23 11:37:27 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Are you willing to try Vue as well?

Nope.
Comment 4 Owen Leonard 2022-02-23 12:04:40 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Tomás Cohen Arazi from comment #2)
> > Are you willing to try Vue as well?
> 
> Nope.

Can you elaborate on your answer?
Comment 5 Jonathan Druart 2022-02-23 12:33:14 UTC
(In reply to Owen Leonard from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > (In reply to Tomás Cohen Arazi from comment #2)
> > > Are you willing to try Vue as well?
> > 
> > Nope.
> 
> Can you elaborate on your answer?

I have no idea how Vue is working and I won't have time to provide another implementation in any other framework/library.
Comment 6 Jonathan Druart 2022-02-23 12:34:41 UTC
And, to be honest, given bug 15522 was written using React, I assumed we agreed on that years ago already.
Comment 7 Owen Leonard 2022-02-23 12:40:16 UTC
Bug 15522 isn't in good shape anymore, and technology changes. I don't know what the right choice is, but thank you for explaining your reasoning.
Comment 8 Tomás Cohen Arazi 2022-02-23 15:27:43 UTC
(In reply to Jonathan Druart from comment #6)
> And, to be honest, given bug 15522 was written using React, I assumed we
> agreed on that years ago already.

I don't have a strong opinion on this subject. I like what I see, and index.js looks like a warning flag to me, for future additions. Not sure how it would work.
Comment 9 Jonathan Druart 2022-02-23 16:01:20 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> (In reply to Jonathan Druart from comment #6)
> > And, to be honest, given bug 15522 was written using React, I assumed we
> > agreed on that years ago already.
> 
> I don't have a strong opinion on this subject. I like what I see, and
> index.js looks like a warning flag to me, for future additions. Not sure how
> it would work.

It does not need to be called index.js, if you look at it it will only deal with cities code.
Comment 10 Fridolin Somers 2022-02-23 19:16:21 UTC
Great :D

It adds :
koha-tmpl/intranet-tmpl/prog/js/src/cities/DeleteCity.js
koha-tmpl/intranet-tmpl/prog/js/src/cities/AddCity.js
koha-tmpl/intranet-tmpl/prog/js/src/cities/CityList.js

May we rename cities/ListCity.js ?

My 2 c.
Comment 11 Tomás Cohen Arazi 2022-02-23 19:17:23 UTC
(In reply to Fridolin Somers from comment #10)
> Great :D
> 
> It adds :
> koha-tmpl/intranet-tmpl/prog/js/src/cities/DeleteCity.js
> koha-tmpl/intranet-tmpl/prog/js/src/cities/AddCity.js
> koha-tmpl/intranet-tmpl/prog/js/src/cities/CityList.js
> 
> May we rename cities/ListCity.js ?
> 
> My 2 c.

cities/delete.js
cities/add.js
cities/list.js

?
Comment 12 Michael Hafen 2022-02-23 21:41:05 UTC
(In reply to Jonathan Druart from comment #0)
> 
> We were expecting to see different implementations in React or Vue, but none
> came.
> 
I'd like to point out Crank.js as an option as well.
https://crank.js.org/

It's similar in purpose to React, but uses promises and generators.
Comment 13 David Cook 2022-02-23 22:47:53 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Are you willing to try Vue as well? I fear index.js will grow too much when
> adding more components. I'm not on either camp but I remember Vue was better
> suited for modernizing legacy projects that don't intend to get converted
> into a full-fledged SPA.
> 

In August 2020, I wrote a Koha plugin that used Vue.js, and it was really simple to work with it. Admittedly, all it did was use Vue.js to render a "Hello world" and then use a button to reverse that message on demand, but the same approach could be used for CRUD with cities. I wrote the HTML in a "vuejshelloworld.tt" file, and included my Javascript in a "jsinclude" block. It was very similar to how we do things now, except more streamlined. 

Earlier this week, I started getting back into coding on my own time (and not just work time), and one of the things I do want to look at is Vue.js, React, SPAs, and all things front-end...

However, my current "fun project" is a non-Koha backend Golang project, and I want to get that done first...
Comment 14 David Cook 2022-02-23 23:01:36 UTC
Also, I just wanted to say kudos to Jonathan for doing this. It's something I've been wanting for Koha for a long time, and I think it's awesome that he's stepped up to do it!

I haven't tried the patch yet and I haven't looked at React in years, so I think I'd need to do some reading/exploring before I could comment well on this code.

At a glance, it seems like a lot of code to do CRUD, but that might just be the React way. I've noticed with DSpace 7, which uses Angular Universal, that some of these Javascript frameworks can take a lot of code to do small tasks...

Anyway, more feedback to come in future once I explore more. Thanks, Jonathan!
Comment 15 Martin Renvoize 2022-02-24 10:25:05 UTC
I too would love to see a Vue version side by side with this.

The code looks nice enough to me here, though I'm by no means an expert in any of the JS frameworks.  I think React has come a long way since I last looked at it as this looks a little more elegant at first look.. though it does feel like a lot of boilerplate.  

For me, React and Vue are the two contenders.. we've already ruled out Angular I believe as it's just too 'build from scratch' for our needs and given our slow adoption of these things I feel we should stick to the two biggest/most popular frameworks of the moment with tried and tested long term records.  Thus Vue vs react for me.

I'll have a play with this and see how it feels..
Comment 16 Marcel de Rooy 2022-02-24 10:36:23 UTC
Very interesting indeed. Thanks for testing this, Jonathan.

I am not very familiar with both tools, but iiuc React needs more skills than Vue while React has a larger community and some Vue documentation is only in Chinese :) ?

Would choosing a tool that might create a barrier for developers to join us, eventually be a problem in the long run?
Comment 17 Martin Renvoize 2022-02-24 11:36:59 UTC
(In reply to Marcel de Rooy from comment #16)
> Very interesting indeed. Thanks for testing this, Jonathan.
> 
> I am not very familiar with both tools, but iiuc React needs more skills
> than Vue while React has a larger community and some Vue documentation is
> only in Chinese :) ?
> 
> Would choosing a tool that might create a barrier for developers to join us,
> eventually be a problem in the long run?

That is precisely my concern.  Having said that, I think there are allot of Vue and React developers out there.. Finding new devs will likely become easier once we adopt one of these.. maintaining some of the existing ones may be more difficult.. I think we need a lean learning curve, which is what drew me to Vue (But I've barely scratched the surface of tutorials yet myself).  We want something that won't exclude the likes of Owen or Katrin for example.

I wasn't aware that some Vue docs are only in Chinese.. everything I've come across has been in english.
Comment 18 David Cook 2022-02-24 23:35:49 UTC
(In reply to Marcel de Rooy from comment #16)
> Would choosing a tool that might create a barrier for developers to join us,
> eventually be a problem in the long run?

Most new developers I encounter have learned React at university, and a lot of external developers have a lot more experience with modern technologies than Koha developers, so I don't think that we need to worry about that too much. 

I've had co-workers refuse to work on Koha, because it's an intimidating mess of old Perl CGI code. Even if a developer were unfamiliar with the Javascript framework we use, it would probably be faster for them to learn it than for them to learn Koha's Perl code. 

--

My hope would also be that we would create re-usable components, so that there wouldn't be much work to do for adding a new DB table and its associated CRUD interface.
Comment 19 Katrin Fischer 2022-02-26 13:02:39 UTC
Thanks for thinking about the learning curve and how to include new devs. I think as a project we have a bit of a mix of people with very different experience levels. With each new tool we adopt, we add to the learning curve.

Reading an intro to Perl and SQL is no longer enough, there is a lot on top that people need to deal with coming to the project to fix a bug/make a change. One of the greatest charms of Koha is the fact, that everyone can make a difference. But the difficulty level has certainly risen in terms of coding/programming in the last 10+ years. It's something that is worth keeping in mind.

I am not argueing against adopting Vue or React, but I think we need to include these things in discussion, have early-on and updated documentation, updated coding guidelines etc. Overall I think key of being more attractive to new devs is going to be good developer documentation.
Comment 20 Fridolin Somers 2022-02-28 06:23:57 UTC
Great :)

Code is really easy to read and understand.
There are good points.
Surely choosing "the" JavaScript framework is a hard choice.
Comment 21 Jonathan Druart 2022-02-28 16:17:31 UTC
Created attachment 131169 [details] [review]
Bug 30160: Don't use classes but hooks

The modern way to use react is to not use classes but functions, as well
as useEffects and useState
https://en.reactjs.org/docs/hooks-intro.html
Comment 22 Jonathan Druart 2022-02-28 16:20:29 UTC
(In reply to Jonathan Druart from comment #21)
> Created attachment 131169 [details] [review] [review]
> Bug 30160: Don't use classes but hooks
> 
> The modern way to use react is to not use classes but functions, as well
> as useEffects and useState
> https://en.reactjs.org/docs/hooks-intro.html

This new version is easier to read but I am getting the following warning I don't manage to get rid of: "Warning: A component is changing a controlled input to be uncontrolled."
Comment 23 Jonathan Druart 2022-03-04 10:48:35 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Tomás Cohen Arazi from comment #2)
> > Are you willing to try Vue as well?
> 
> Nope.

Or maybe yes. See bug 30225.
Comment 24 Katrin Fischer 2023-06-17 15:47:09 UTC
As we've settled on Vue, I believe this can be closed.