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

Migrate to React #5

Open
heberleh opened this issue Dec 16, 2021 · 11 comments
Open

Migrate to React #5

heberleh opened this issue Dec 16, 2021 · 11 comments

Comments

@heberleh
Copy link
Owner

heberleh commented Dec 16, 2021

Interactivenn was written in poor and simple Javascript.
Many functions are duplicated, etc.

React can be used to manage the state of the App, interactivity and inputs, which in turn call the Services to deal with the data.

Because of flexibility, webpack should be used directly, that is, avoid using create-react-app. This will give us flexibility to generate web and module distributions. I believe it may help in a slow migration to Typescript #7 too.

@jayeclark
Copy link

I started taking a look at this and would love to help with this issue. I actually started the migration process and got fairly far along before I realized the better approach would be to refactor the JS to modules first, before adding React or Typescript into the mix.

@heberleh
Copy link
Owner Author

heberleh commented Jan 4, 2022

I was thinking about creating a component Diagram(sets, unions) that is then used on the website and exported as a module as well.

For example, a module that, one can easily generalize the React component into a function call like this:

import interactivenn from "interactivenn";

interactivenn.createDiagram(div, sets, unions);

This would make the interactivenn a library that can be used elsewhere.
With the module, a few new doors open to increase the access of the tool to other systems as well.

Most of the logics for the module would be in what I called "services", that will create diagrams, calculate unions, intersections, etc.

I created a new branch with a template of my idea, perhaps you may consider it:
https://github.com/heberleh/interactivenn/tree/typescript

I read that you only have a short time to work with this project in your profile, so I also created this issue, which would be a simplification of the current one: #7

The current web interface is 'fine', since it is intuitive and simple enough for most users, but it can for sure be improved.

Let me know if you end up coding something, and also if you are interested in scientific publication (in case we work more on this project). I will try to make time to code if this project is reborn. Having a module sounds exciting!

@jayeclark
Copy link

It took a lot less time than I thought it would to do the heavy lifting prep work (converting all the js to run as modules rather than direct imports).

I should be able to convert the site to use React components in the morning. Once I’m done with my challenge (less than 10 PRs away now) I can circle back and do some additional refactoring and refinement.

@jpmoura
Copy link

jpmoura commented Jan 9, 2022

I would like to help with the issue #7 and would be nice if this PR was merged before I start to work on it @heberleh.

I'm also a USP ICMC alumnus, so be able to help with this project is special to me.

@heberleh
Copy link
Owner Author

heberleh commented Jan 9, 2022

@jpmoura I'm glad to hear that!

@jayeclark does it make sense to wait the complete PR or partial? Please let us know what you think.

I think the code is becoming much more modularized now.

@jpmoura I wrote typescript but I'm not sure if it will already be in typescript when @jayeclark is finished with the contributions. Regardless of it, any contribution towards the more reusable/modularized/cleaner code is very welcomed! In JavaScript or Typescript.

P.S. I visited Portugal this year, I'm already missing the sun! 😁

@jayeclark
Copy link

@heberleh Seems like #7 could be started on as soon as that first step that I mentioned in the draft PR thread (converting the diagram svg into a component.)

I’ll post more details later today, but I think I’ve come up with a good, modular migration plan and, if you agree with the approach, I could open a first PR for that plan against the develop branch as early as tomorrow afternoon.

@heberleh
Copy link
Owner Author

heberleh commented Jan 11, 2022

@jpmoura I think you can contribute now if you want. Please read this to get some context: #19

Please let us know what are your ideas and plans.

Some steps towards the SetsService are to move some remaining functions to that class, as pure functions,
from here https://github.com/heberleh/interactivenn/blob/dev/diagram/Diagram.js:

  • getIntersectionsSet() - rename to getIntersectionsHash? or getIntersections?
  • mergeSets()
  • validateUnions() - this is probably printing a warning to the user and may fit another class... that is connected to UI. This function can be split into others that should be in the SetsService, like isListOfUnionCodesValid(list of codes) and create isUnionCodeValid(code). Then validateUnions can call isListOfUnionCodesValid, and if false, show modal with error message.

Another basic thing that is currently dirty: each set itself could be a data structure, with id (A, B, C... ) and elements;
Rather, elements should be of type Set or Array can be analyzed.
And sets perhaps a hash or something like that.

getPercentageString can be moved from SetsService to some other Service that takes care of "UI" things, or simply util.js.
It's used to print the number on the diagram.

Please don't hesitate to give ideas and propose what you would like to code, being it in this list or not.

@heberleh heberleh changed the title Migrate to Typescript + React Migrate to React Jan 12, 2022
@heberleh
Copy link
Owner Author

heberleh commented Jan 12, 2022

Updated title and description of #5 and #7 to better separate ideas.

@jpmoura
Copy link

jpmoura commented Jan 28, 2022

@jpmoura I think you can contribute now if you want. Please read this to get some context: #19

Please let us know what are your ideas and plans.

Some steps towards the SetsService are to move some remaining functions to that class, as pure functions, from here https://github.com/heberleh/interactivenn/blob/dev/diagram/Diagram.js:

  • getIntersectionsSet() - rename to getIntersectionsHash? or getIntersections?
  • mergeSets()
  • validateUnions() - this is probably printing a warning to the user and may fit another class... that is connected to UI. This function can be split into others that should be in the SetsService, like isListOfUnionCodesValid(list of codes) and create isUnionCodeValid(code). Then validateUnions can call isListOfUnionCodesValid, and if false, show modal with error message.

Another basic thing that is currently dirty: each set itself could be a data structure, with id (A, B, C... ) and elements; Rather, elements should be of type Set or Array can be analyzed. And sets perhaps a hash or something like that.

getPercentageString can be moved from SetsService to some other Service that takes care of "UI" things, or simply util.js. It's used to print the number on the diagram.

Please don't hesitate to give ideas and propose what you would like to code, being it in this list or not.

Sorry that I took so much time to answer, but I got too many issues to fix in the last couple of weeks.

There's no much to explain because the goal is really straightforward: migrate pure functions (adding unit tests) and grouping them in meaningful classes.

I saw that you created services classes to put some "generic" functions inside them. Are you thinking in data structure classes as only a data bag, or can we add the functions in them to be more adherent with object-oriented approach?

Are you expecting more of a DDD or a Clean architecture?

@heberleh
Copy link
Owner Author

heberleh commented Feb 4, 2022

@jpmoura I wouldn't use OO here, but would create Types for the data. I'm imagining that the data is clear 1-6 Sets, and those have id, elements, what else the app needs. The idea of Services is that, although classes, you export 1 object, so it is a global variable that is called like in service.paint(canvas). The functions should be pure functions.

Reasons for pure functions and services is that lots of code here can be independent of the UI and Vis. UI can also be separated from Vis (main diagram). If you think of a hierarchy, we have these "boxes": full-website > diagram+interactivity+input_boxes > diagram+interactivity > diagram-only.

E.g., diagram+interactivity (union operations, mouse-over, export button, ...) is independent because I could pass the Sets by coding, if this was exported as a package.

I'm not sure about architectures, I'm not a software engineer actually—usually I just work with some prototypes of visualizations or some AI stuff.

heberleh added a commit that referenced this issue May 29, 2022
@heberleh
Copy link
Owner Author

In the dev branch, I added an interactivenn folder with a basic idea of the new structure of the project in Typescript with React and npm using create-react-app.

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

Successfully merging a pull request may close this issue.

3 participants