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

Add web documentation #201

Merged
merged 2 commits into from Jan 1, 2023
Merged

Conversation

aitorllj93
Copy link
Contributor

@aitorllj93 aitorllj93 commented Dec 26, 2022

As discussed here I have been working on improving the docs. Here's a first approach by using Docusaurus

Landing Page:

image

Core Concepts (+ dark theme which I personally prefer):

image

image

Auto-generated pages for Values and Nodes (for Core and Scene profiles):

image

image

Auto-generated pages with the Examples found in the graphs folder:

image

Easy to use blog in case we need it (can be disabled if there's no plans to use it):

image

That said, there are some troubles I found during the development and should be discussed:

  • Many different interfaces for Nodes (NodeDescription, NodeDescription2, Node Instance, NodeSpecJSON) and some misalingments between them. It would be nice to be able to generate the Node pages just with the NodeSpecJSON but right now it's not possible:

    • writeNodeSpecsToJSON fails when executing on a Registry with just the Scene profile so I need to load 2 registries, to be able to check which Nodes are from each Profile
    • the NodeSpecJSON doesn't include some fields such as the helperText which in this case it's quite useful
    • the NodeSpecJSON doesn't include any information about dynamic I/O as they are defined with the new configuration property, so I have to do really weird tricks to do this. Again, I think would be nice to be able to determine this based only on the NodeSpecJSON, this is the main interface for the Node definitions on the reactflow implementation
  • The reactflow component has some issues that makes it hard to reuse it right now:

    • The styles are defined by some kind of css class utility library (maybe Tailwind? Idk) so in order to use them inside the docs I would need to find/redefine them and load them into the blog and also taking the risk to arise conflicts. I would favor some styles-in-js implementation such as emotion.
    • The presentational components are very tied to the reactflow engine (mainly because of the hooks they're using) so I hard to rewrite the entire node inside the docs
    • The runner function is defined deep inside the components so there's no way to override it with custom profiles rn

As we're having many changes on the core interfaces (NodeDescription) and the way we define the nodes, I don't think it makes sense to update the reactflow component till those changes are completed. But once we have polished those details, I would like to improve the visual editor so we can integrate it inside the docs and provide a visual execution environment for the examples section.

Also, we should include a GH Action to deploy the website to GH pages. I can work on that before we merge this PR and include it with the changes.

cc @bhouston @oveddan

@aitorllj93
Copy link
Contributor Author

aitorllj93 commented Dec 26, 2022

forgot to mention, if you want to run the project locally to give it a try, just checkout the branch, move to the website folder, run npm install and then npm start.

If you want to regenerate the dynamic content (values, nodes and examples) just run npm run generate-dynamic-pages on the same website folder

@oveddan
Copy link
Collaborator

oveddan commented Dec 26, 2022

Wow this is so awesome!!! Thanks for doing this. Will take a review soon.
Would be also cool to in the future add a storybook

@oveddan
Copy link
Collaborator

oveddan commented Dec 26, 2022

@aitorllj93 I believe the work extracting the definitions as interfaces #200 will help with the issues around getting the node definitions to generate documentation.

@oveddan
Copy link
Collaborator

oveddan commented Dec 26, 2022

@aitorllj93 Do we need all these auto-generated pages checked in? Can't they just be made at build time? Or is there a way to build less files?

@aitorllj93
Copy link
Contributor Author

@aitorllj93 Do we need all these auto-generated pages checked in? Can't they just be made at build time? Or is there a way to build less files?

@oveddan I removed all of them so now there are less code changes. If you already did checkout of the branch keep in mind I did reset the commit as it was easy to add the ignore before including the files so you might need to reset hard the branch with origin instead of git pull

@aitorllj93
Copy link
Contributor Author

@aitorllj93 I believe the work extracting the definitions as interfaces #200 will help with the issues around getting the node definitions to generate documentation.

Cool, I'll take a look on it and tell you if I find any issue after that PR is merged

@aitorllj93
Copy link
Contributor Author

aitorllj93 commented Dec 26, 2022

@bhouston @oveddan Regarding the dynamic I/O here are the tricks I had to use in order to be able to identify them:

First I had to instantiate every single node with numInputs and numOutputs setted so I can get them from the Node instance, also had to initialize the customEvents and variables properties because otherwise some nodes were crashing:
image
Then I had to compare the Node instance which has the dynamic nodes already setted with the NodeSpec which doesn't have them so I can figure out if that Node has dynamic I/O. This only works for numeric cases
image

That's the reason I think the dynamic I/O specification is incomplete with just the numInputs/numOutputs configuration, I think we should be able to determine before the Node gets instantiated:

  • whether a Node can have dynamic I/O or not (this is currently doable with the configuration definition but only if the Node has a single case for each, ie. it's not possible to define a Node with n text inputs + n flow inputs)
  • which types will those sockets accept
  • and maybe which names do they accept (numeric, text...)

Right now this is a must not only to be able to provide a better logic for docs generation but for the visual editor to be able to recognize them as well

It would be great to have something like this in the NodeSpec:

dynamicInputs:
  flows:
    valueType: "flow"
    keyType: "integer"
    maxAmount: 5
    minAmount: 1
  numbers:
    valueType: "integer"
    keyType: "text"
    maxAmount: 10
dynamicOutputs:
  results:
    valueType: "text"
    keyType: "text"
    minAmount: 1

Or maybe considering the current NodeSpec implementation we can extend the inputs & outputs types to allow it:

inputs: 
  - name: flows
    valueType: flow
    isMultiple: true
    keyType: integer
    minAmount: 1
    maxAmount: 5

And just query by the isMultiple/isDynamic (Idk how to name it) prop

In order to use them inside the Node we could just get the "group" by the name and iterate over them:

const textInputs = readInput<DynamicInput<string, string>[]>('texts');
const inputText = textInputs.find(t => t.name === 'myTextInput').value;

const flowSockets = this.outputSockets.find((s) => s.name === 'flows')?.sockets;
const specificFlowSocket = flowSockets.find(s  => s.name === 'mySwitchCase');

@aitorllj93
Copy link
Contributor Author

I tested the automated deployment with GitHub Actions in a personal repo and it works like a charm. @bhouston after the PR gets merged and the gh-pages gets created by the workflow, you'll need to enable it on the repository settings:
image

@bhouston
Copy link
Owner

bhouston commented Jan 1, 2023

This is phenomenal stuff! Let me merge it and see if I can get it to deploy correctly.

@bhouston bhouston merged commit b5179ae into bhouston:main Jan 1, 2023
@bhouston
Copy link
Owner

bhouston commented Jan 1, 2023

Looks like the build failed, probably because it wasn't based on the new changes that @oveddan did? https://github.com/bhouston/behave-graph/actions/runs/3817063736

@aitorllj93
Copy link
Contributor Author

aitorllj93 commented Jan 1, 2023

Looks like the build failed, probably because it wasn't based on the new changes that @oveddan did? https://github.com/bhouston/behave-graph/actions/runs/3817063736

Sure it is, I need to update the docs (some of the examples I added to the documentation are using the old way to define nodes) and make the dynamic generation for node profiles working with the new interfaces, I'll try to get some time during next week or weekend to fix it

@oveddan
Copy link
Collaborator

oveddan commented Jan 1, 2023

@aitorllj93 could you also make sure that you are importing the other packages within website as:

import {
  registerCoreProfile,
  registerSceneProfile,
  Registry
} from '@behave-graph/core';

and not using relative paths? You can see how the examples do it.

@aitorllj93
Copy link
Contributor Author

@oveddan after checking the code I noticed some things are failing with the code in the main branch:

Outdated configuration on the CustomEvent nodes:
image
image

Error in nodeFactory > makeCommonProps:
image

I'm not sure how to fix the last one, can you please check it?

@aitorllj93 aitorllj93 mentioned this pull request Jan 16, 2023
@oveddan
Copy link
Collaborator

oveddan commented Feb 19, 2023

@aitorllj93 this has been fixed in #211

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.

None yet

3 participants