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

WIP Feature/smart form #31

Merged
merged 79 commits into from Jun 8, 2021
Merged

WIP Feature/smart form #31

merged 79 commits into from Jun 8, 2021

Conversation

eric-burel
Copy link
Collaborator

@eric-burel eric-burel commented Apr 7, 2021

To do

Step 1: get the form to work with Vulcan API

  • Document a bit by creating a readme
  • In packages/react-components/components/form/Form/fields.ts, move the side effect of handleFieldPath (default value setting) somewhere else, to make the function pure
  • Pass callbacks through context, provides hooks etc. => ok for core components of the form. To be done for inputs, not sure of the best strategy, because that means needing a huge refactor. We could provide those values both through props and let the user also use the context?
  • Provide doc on how to create your own components
  • We need to figure out fragment registering. Some parts of the Form are relying on "expandQueryFragments" from "packages/vulcan-lib/lib/modules/fragments.js". What to do with those?
  • Get "Locales" from context in packages/react-components/components/form/FormIntl.tsx
  • Correct props for the context that provides the components
  • Correct input props, whitelist correctly when using an HTML input => started the work, but we still to correctly differentiate "HTML INPUT" and "HTML SELECT" correctly
  • Figure out "beforeComponent" / "afterComponent", and "instantiateComponent" pattern
  • Reintroduce existing unit tests => we could use this as an occasion to use the new storybook testing plugin, to load the form from stories directly? So that we don't duplicate decorators and all
  • Implement the SmartForm, that queries actual graphql (=FormContainer.tsx) => ongoing work, started to create the stories
  • Fix Storybook Testing React misconfiguration setGlobalConfig not defined storybookjs/testing-react#27
  • Fix issue with circular dependency: in packages/react-ui/components/form/tests/Form.stories.tsx, when importing ./Form, we have a circular dependency with packages/react-ui/components/form/defaultVulcanComponents.ts, not sure how to fix it... Currently we hard copy the Form folder to Form2 to duplicate the components but that's not very good.... => solution is to split the context provider with default component from the context consumer with the hooks, this avoids having an explicit circular dependency
  • Figure how to set the context => Story ok, nested dependencies seems to work ok
  • How to handle depencies between components that belongs the same context? => OK
  • Handle getLabel context, eg in packages/react-components/components/form/FormError.tsx
  • Switch to TS
  • Switch to stateless => done
  • Finish the FormLeavingManager component to handle the warnOnUnsavecChanges feature (we need to figure what is the history object exactly + stop triggering the message on page leave. We might need to recode the history.block feature?
    How to intercept route changes? vercel/next.js#12348
    https://github.com/ReactTraining/history/blob/master/docs/blocking-transitions.md
    Added beforeRouteChangeStart vercel/next.js#5377
    Route cancellation vercel/next.js#2476 (comment)
    Browser Back Button does not trigger window.onbeforeunload vercel/next.js#2694
    => ok for browser events. See Listen to route changes in a SPA without tight coupling framework specific router implementation #40 for SPA specific routing, this belongs to Vulcan Next

image

Step 2: improve the API: better naming, clearer components structure etc.

  • Move graphql logic of the success callbacks out of Form, and put it into FormContainer

Others

About custom components

It is already possible to use a custom component as input:

{
   type: String,
   input: MySuperReactInput
}

Limitation is that you cannot override this component using the context, it's hard written in your schema.

We could allow something like this:

{ 
   type: String,
   input: "MyCustomInput"
}

The advantage of this pattern is that you can change "MyCustomInput" from the context, as other Vulcan inputs. It can be interesting if you want to implement an API that allows some component swapping or programming a multitenant app.
You basically can create your own Vulcan app.

Limitation is that this pattern, which already exists in Vulcan Meteor, tends to be deadly over-used: since the reference to the component is implicit, you may unknowingly create circular dependency, reference a component that doesn't exist etc.
So I am not in a hurry to add this feature. This might need another ticket.

@eric-burel eric-burel marked this pull request as draft April 16, 2021 16:09
@eric-burel eric-burel merged commit 12b8159 into devel Jun 8, 2021
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

2 participants