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

Refactor site to react #8

Closed
wants to merge 7 commits into from

Conversation

jayeclark
Copy link

Partially fixes #5

This is turning out to be quite an undertaking! Creating a Diagram component that executes most updates through state management required quite a bit of refactoring. As it stands, this is a massive PR (though, it's not as large as it looks - most of the additions/deletions are from functions being copy/pasted from one large file into their individual components.)

I've gotten pretty far along on implementing a pure-javascript React component for the Diagram. This means it can be served on a regular server with no build step. I wondered what you thought about converting fully to React (i.e. using create-react-app or manually setting up a similar configuration.) That would make the code easier to read & simpler, but would require and extra build step in order to serve the site.

I have a prototype running here. There are still some bugs I need to work out with the merge scripts for Unions by Tree, but most everything else seems to be working.

I'll likely open a separate PR to take care of some bugs & styling issues that I found & was able to resolve in the current code (issue #3 ). I have ~1 week left of work on my 100 PR challenge so I need to focus on that for a bit, but I did want to open a draft PR for now for feedback on:

  • Thoughts on the best way to structure the process of transitioning to React. Reviewing 4700+ insertions and deletions does not seem feasible! I just have them all in here so you can see the different commits & general approach.
  • Whether I should go for a full React app or stick with the 'lite' version using js only (no jsx) and importing React scripts into index
  • Any other issues with the user interface other than the known ones (Unions by Tree merging is not fully migrated yet & breaks)

@heberleh
Copy link
Owner

heberleh commented Jan 8, 2022

Thank you so much for the effort put into this project! It seems already much better!

I agree that having the build step nowadays is inevitable, and it was my original idea: having the code more modularized and perhaps written in React and Typescript. I created at my job a webpack file that creates a dist/web and a dist/modules.
In web, the website is stored and can be served. In the modules, a package is available, from which users can call the functions and use the system into other systems/websites. E.g., package.createDiagram(div, set, options).

Regarding the best way to migrate to React, I can't say. Maybe a top-down approach may help?

Before I explain some of my ideas, let'me explain what I mean by Diagram: the venn diagram graphic. So, a component Diagram, would be responsible to have an input and outputs an SVG. For example, would render .

I think the names of what we are calling diagram and "setsImage" is confusing. For example, rather the user selects "lists" or "tree", this is only relevant for the Website, not for the diagram. Venn diagrams will always look the same. They can be 2-way, 3-way, ... 6-way. Tree vs. Slide is only a matter of how the user writes the union operations that happens when they click the "slider" buttons < >.

So, what I see that could be done next, if you would like to, is something like this:

  1. separate concept of Website, InteractiVenn and Diagram. The system would have components like this, not necessarily with these names <App/>, <Header/>, <SetInput/>, <SetsInputs/>, <InteractiVenn/>, <SliderList/>, <SliderTree/>, <Diagram/>.
  2. Website and interactivity would be managed using React.
  3. Website would be the top bar, the input boxes and the InteractiVenn, and some other things.
  4. InteractiVenn would be the result after the user inputs the sets... it would include the sliders, the diagram, maybe the export menu, etc.. The subcomponents could be hidden on demand (flags true/false).
  5. A component C could control rather sliderList or SliderTree is displayed inside <InteractiVenn>.
  6. So InteractiVenn could be later exported to the modules... so someone could use the Diagram and the unions in somewhere else, like in a Jupyter notebook. And they could choose if some elements are displayed or not, like where they write the list of unions "ab;ac;de". These details I can implement later... these flags-true-false.
  7. <Diagram> would render SVG exclusively, which can also be exported as module/function later too.
  8. Later I can create this module with functions like createInteractiVenn(div, sets, opts) and createDiagram(div, sets, opts), which would append the interactive
    or the in the given div. The functions I can create in a way that they render the React components <InteractiVenn> and <Diagram>, given the div.

Current bugs that I found:

  • export button should be png, svg, and txt (txt I think it exports the intersections)
  • sometimes the union doesn't work. E.g., I set the 6-way diagram, then I click start union. Then I stop, and change to a 3-way diagram, and then I try to do the unions and it doesn't work when I click < >. Note that if you input 6 sets and click (3), it should show the 3 first sets, and if I click (6) it should show all 6 sets again, so the current behavior is correct).
  • One time (I can't reproduce) it said it couldn't find 6waydiagram_.svg, which doesn't exist. Correct is 6waydiagram.svg. I imagine that in the "unions" but it identified a union formed by "" (none) and tried to find a set with that union, so it appended with 6waydiagram_.svg

I created a main branch, copy of master, and I should change the branch where you push to dev. I might work on it a bit before I push to the website. And I probably will need to learn how to push it to google cloud, or, I should create GitHub actions that build, and then it pushes just the plain dist/web to google cloud automatically (if this can actually be achieved, I never used the actions yet).

Many thanks for all the work you put into this project already! It has already reshaped it so much! Thank you!

@heberleh
Copy link
Owner

heberleh commented Jan 8, 2022

A new tab Contributors would be good to have too. We can add your name there and if you like, a URL to some of your profiles/website.

@jayeclark
Copy link
Author

Thanks so much for the feedback. I think I am already on the same page about a lot of things, but I understand much better what you mean about diagram being separated out as a module from the site. I’ll think more about the best way to implement that as I’m finishing up the last handful of PRs in my challenge. Most likely I’ll start from scratch and focus on modularizing the diagram svg first. That should be a relatively small PR to start as opposed to this one! Creating a dev branch to open PRs against would be great as well.

I can probably also help a bit with the GitHub actions (as well as setting up preview deploys for PRs)

@heberleh
Copy link
Owner

heberleh commented Jan 8, 2022

I tried to change the default branch to main and I was about to rename master to dev, but Github said it can have bad consequences 🗡️ , so I just created a dev as well. Feel free to use any of them, and later I clean up.

@jayeclark jayeclark changed the base branch from master to dev January 9, 2022 18:10
@jayeclark jayeclark mentioned this pull request Jan 9, 2022
@jayeclark
Copy link
Author

OK, I've thought about this some more and it definitely makes sense to work only on the Diagram component first. But, the way I'm envisioning it, that would include:

  • the SVG itself
  • the interface that allows downloading of the SVG
  • the interface that allows adjustment of the font size and opacity (over time other features could be added to the display options, for example a simplified color picker, enabling basic dragging and dropping of labels, sizes, and elipses, etc)

The Diagram component would accept three props: data, config, and output. Data could be either a string (in the .ivenn format) or a json object that contains the same information. Config would be an object containing options like global font size, opacity, colors, etc. that modify the defaults. Output would be passed as an object with four properties: type* (svg, png, htmlElement), svgOptions, pngOptions, and htmlElementOptions.

Each output type would have optional configuration options that can override the defaults. For example, the output object might look something like this:

output: { 
  type: "htmlElement",
  htmlElementOptions: {
    showExport: true,
    showDisplayControls: false,
  }
}

or this:

output: { 
  type: ".png",
  pngOptions: {
    backgroundColor: white,
  }
}

In terms of actually implementing this, I think it makes sense to add a new folder, at the same level as /web, and spend a few commits copying over the relevant code. The first commit would be just a direct copy-paste of relevant existing code, with no changes. The second would move things around into a better file structure - but again, not changing any code. This will obviously break everything, but having already broken it all already (hehe) I should be be able to fix it all again fairly quickly and do so in a series of commits that make sense. And this way it will be much much easier to follow along with the changes than the way I originally did it, since the first two commits establish the baseline.

I can also implement an extremely basic index page in that new folder for testing (it's certainly possible to write 'blind' tests as well, but sometimes visual tests are so much faster to determine that something has gotten messed up.) The index page would only take 3 inputs - a file with the data, plain text config code, and plain text output code - and have one area to display the output.

Let me know what you think of this approach!

@heberleh
Copy link
Owner

heberleh commented Jan 10, 2022

How about something like this?

<App>
...
<Interactivenn>
   <Slider/>
   <Diagram/> (pure SVG)
   <BottomMenu/> 
</Interactivenn>

<Input/>
...
</App>

If in the future I export Interactivenn as a package for Python, Slider and the BottomMenu can be optional.
So, the user can show only Diagram, only Diagram and Slider, or only Diagram and BottomMenu, or all.

Cases:

  1. User only wants a venn diagram;
  2. User wants a venn diagram and to perform unions;
  3. User wants to do 1, but change the opacity
  4. User wants to do 2, and export one or the other

Users could still setup things using the parameters of the Python object that creates and controls the diagram. So maybe they want only to show the diagram. Or an interactive diagram (diagram + slider), and so on.

Actually, something like this could work too because Input could be by default hidden in the Python version... and displayed only if the user wants, although in Python users would tend to upload data using the arguments of a function.

<App>
...
<Interactivenn>
   <Slider/>
   <Diagram/> (pure SVG)
   <BottomMenu/> 
   <Input/>  ***
</Interactivenn>
...
</App>

For the python/R libraries, I would port only the <InteractiVenn/> component.

@jayeclark
Copy link
Author

Sure, that makes sense for how the React wrapper would work, but it would be much more versatile to have the output of the diagram module be framework-agnostic so that it can be used in React, Vue, Angular, any framework or even vanilla HTML/JS. Similar to the distinction between Bootstrap and react-bootstrap. (Or D3 and Britecharts.)

@heberleh
Copy link
Owner

heberleh commented Jan 10, 2022

So, I was thinking about having DiagramService that has functions that do: create and return svg element, create and append svg to div, etc.

Then this can be called in the component, so it would still be a component that calls this other "pure" js/ts class/function.

The entire React components with menus, as well as a simple createSVG function, can be later exported in a module/package, that I can use to build Jupyter and R plugins, for example.

Actually, once an npm package is created, it is in plain JavaScript, right?, and can be used anywhere. So the "pure" js may not be required.

For example:

var interactivenn = require('interactivenn');
import interactivenn from 'interactivenn';

var div = document.getElementById("diagram-container");
interactivenn.appendDiagram(div, sets, options);

var svg =  interactivenn.createSvgDiagram(sets, options);

var canvas = interactivenn.createCanvasDiagram(sets, options);

interactivenn.appendInteractiveDiagram(div, sets, options);

This would make it very flexible.
I know that when I used a package with React in a project that used React, it was complaining about versions. But I think it only complained because React was set as dependency and not peer-dependency. Something like that. But the idea is that everything should be transpiled to plain JS and a set of functions is exposed through the package.

We could think about having specific packages, like for React, but maybe just making functions more flexible can solve the problem? Not sure how a "const reactElement = createInteractiVennReactElement()" would work.

@jayeclark
Copy link
Author

Yes, that's right - npm packages are pure, transpiled JS. I think we're on the same page now - DiagramService will accept two inputs - an object of data sets and an object with configuration options. It will output an svg. The various ways to manipulate the appearance and content of the diagram will be controlled through other functions/components.

@jayeclark
Copy link
Author

Closing this out in favor of a step-based approach as discussed in #17

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

Successfully merging this pull request may close these issues.

Migrate to React
2 participants