-
Notifications
You must be signed in to change notification settings - Fork 57
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
[3758] Store all form payload in a context #3844
Conversation
20af62b
to
b8f0367
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also rebase the PR to merge it quickly after that, thanks
/> | ||
</div> | ||
))} | ||
const renderedForm = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please stop with the useMemo
memo
, useCallback
etc unless we have a performance issue identified that needs to be fixed (i.e. a Github issue). Premature optimization does not help and React 19 is supposed to provide that for free so we will have to remove this kind of code quite soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed worth it to me there, but I also took the wrong habit to use these too much.
I'll remove it there.
|
||
const onComplete = () => setState((prevState) => ({ ...prevState, complete: true })); | ||
|
||
const { error, loading } = useSubscription<GQLFormEventSubscription, GQLFormEventVariables>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not destructure data here like that { data, error, loading }
and remove the payload from the state? You would also remove one re-rendering with the setState line57 which would disappear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I modified all the custom hook to act like this.
b8f0367
to
f66dfd7
Compare
e0ce686
to
2a130e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take into account the various comments that I've added to this PR and ensure that it gets merge to release the Sirius Web RC1.
packages/forms/frontend/sirius-components-forms/src/representations/FormRepresentation.tsx
Outdated
Show resolved
Hide resolved
...ges/sirius-web/frontend/sirius-web-application/src/diagrams/useDiagramFilterSubscription.tsx
Outdated
Show resolved
Hide resolved
...ges/sirius-web/frontend/sirius-web-application/src/diagrams/useDiagramFilterSubscription.tsx
Outdated
Show resolved
Hide resolved
...sirius-web-application/src/views/edit-project/workbench-views/useDetailsViewSubscription.tsx
Outdated
Show resolved
Hide resolved
...sirius-web-application/src/views/edit-project/workbench-views/useDetailsViewSubscription.tsx
Outdated
Show resolved
Hide resolved
...eb-application/src/views/edit-project/workbench-views/useRelatedElementsViewSubscription.tsx
Outdated
Show resolved
Hide resolved
...eb-application/src/views/edit-project/workbench-views/useRelatedElementsViewSubscription.tsx
Outdated
Show resolved
Hide resolved
...eb-application/src/views/edit-project/workbench-views/useRepresentationsViewSubscription.tsx
Outdated
Show resolved
Hide resolved
...eb-application/src/views/edit-project/workbench-views/useRepresentationsViewSubscription.tsx
Outdated
Show resolved
Hide resolved
Bug: #3758 Signed-off-by: Michaël Charfadi <[email protected]>
2a130e0
to
1edf925
Compare
Signed-off-by: Stéphane Bégaudeau <[email protected]>
I'll wait for the build, merge it and release the Sirius Web 2024.9.0 RC1 (which will be the milestone 2024.7.11) |
Bug: #3758
Pull request template
General purpose
What is the main goal of this pull request?
Project management
priority:
andpr:
labels been added to the pull request? (In case of doubt, start with the labelspriority: low
andpr: to review later
)area:
,difficulty:
,type:
)CHANGELOG.adoc
been updated to reference the relevant issues?CHANGELOG.adoc
? (Including changes in the GraphQL API)CHANGELOG.adoc
? For example indoc/screenshots/2022.5.0-my-new-feature.png
Architectural decision records (ADR)
[doc]
?CHANGELOG.adoc
?Dependencies
CHANGELOG.adoc
?CHANGELOG.adoc
?Frontend
This section is not relevant if your contribution does not come with changes to the frontend.
General purpose
Typing
We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).
useMutation<DATA_TYPE, VARIABLE_TYPE>(…)
useQuery<DATA_TYPE, VARIABLE_TYPE>(…)
useSubscription<DATA_TYPE, VARIABLE_TYPE>(…)
useMachine<CONTEXT_TYPE, EVENTS_TYPE>(…)
useState<STATE_TYPE>(…)
?.
(if the GraphQL API specifies that a field cannot benull
, do not treat it has potentiallynull
for example)let diagram: Diagram | null = null;
)Backend
This section is not relevant if your contribution does not come with changes to the backend.
General purpose
Architecture
Review
How to test this PR?
Please describe here the various use cases to test this pull request