refactor HelpContainer by walesch-yan · Pull Request #1598 · mxcube/mxcubeweb

walesch-yan · 2025-03-19T12:40:51Z

This PR refactors the HelpContainer into a functional component following similar approaches as for previous PRs.

walesch-yan · 2025-03-19T12:48:09Z

+  const links = import.meta.env.VITE_HELP_LINKS?.map((link) => (
+    <div key={link.url}>
+      <a target="_blank" href={link.url} rel="noreferrer">
+        {link.name}
+      </a>
+    </div>
+  ));


Unfortunately the HELP_LINKS were not available in the demo, hence I used the url for the key prop here, it would be best to have an id I guess.

I think this VITE_HELP_LINKS feature is broken since env vars are strings but this one is used as an array in the code. I would consider removing this feature.

For the sake of discussion, storing JSX in a variable should also be avoided (for the same reason that it doesn't reduce the complexity of the component). I would have probably inlined the JSX and maybe stored and parsed the env var in a constant at the top of the file:

cost HELP_LINKS = import.meta.env.VITE_HELP_LINKS; // + parse as array somehow ... // In the JSX: {HELP_LINKS.length > 0 && ( <Card> {HELP_LINKS..map(...)} </Card> )}

marcus-oscarsson · 2025-03-19T13:03:08Z

Nice :)

axelboc

Nice! FYI, I think we considered removing this page at some point, which is why I hadn't touched it yet. Worth making a decision on this before addressing my comments (especially the React Hook Form one 😁 — although it could be good practice if you're interested, since it's such a simple form)

axelboc · 2025-03-19T13:07:26Z

-          </Card.Body>
-        </Card>
-      );
+  const handleSubmit = () => {


I like to use the function declaration syntax for named functions: function handleSubmit() { ... }. I find it helps to distinguish functions from variables/constants when reading the code.

Sure, I guess this was just my personal preference but for the sake of consistency I will change it (and try to not let my inner demon do that again 😆 )

axelboc · 2025-03-19T13:11:03Z

+      <Row>
+        <Col sm={12} className="d-flex">
+          <Col sm={4}>
+            {localContactPanel()}


It's best to avoid "render" functions like this. It doesn't actually remove complexity from the component — on the contrary, it adds a function.

Either inline the JSX or move it into a separate component. In this case, I think inlining is fine — long JSX is okay if the logic (conditions, loops, etc.) is limited.

axelboc · 2025-03-19T13:20:36Z

+  const links = import.meta.env.VITE_HELP_LINKS?.map((link) => (
+    <div key={link.url}>
+      <a target="_blank" href={link.url} rel="noreferrer">
+        {link.name}
+      </a>
+    </div>
+  ));


I think this VITE_HELP_LINKS feature is broken since env vars are strings but this one is used as an array in the code. I would consider removing this feature.

For the sake of discussion, storing JSX in a variable should also be avoided (for the same reason that it doesn't reduce the complexity of the component). I would have probably inlined the JSX and maybe stored and parsed the env var in a constant at the top of the file:

cost HELP_LINKS = import.meta.env.VITE_HELP_LINKS; // + parse as array somehow ... // In the JSX: {HELP_LINKS.length > 0 && ( <Card> {HELP_LINKS..map(...)} </Card> )}

axelboc · 2025-03-19T13:30:25Z

+                  </Col>
+                  <Col className="col-xs-4">
+                    <span>
+                      <b>Mesh </b> <br />


Could you please replace the b and br tags with h3 and remove the span wrappers? Might as well improve the HTML a bit... 😂

Well spotted! Maybe I went a little to fast over this component

axelboc · 2025-03-19T13:33:31Z

+  const senderRef = React.useRef();
+  const contentRef = React.useRef();


Perhaps a good opportunity to try React Hook Form? It's already installed — I used it for the login form and for the manual sample creation form, if I recall.

walesch-yan · 2025-03-19T14:16:15Z

Nice! FYI, I think we considered removing this page at some point, which is why I hadn't touched it yet. Worth making a decision on this before addressing my comments (especially the React Hook Form one 😁 — although it could be good practice if you're interested, since it's such a simple form)

Oh, maybe I missed/forgot this consideration 😅 I guess we can keep it as a draft then, I prefer to use it as a little practice if possible;)

walesch-yan

I added the suggestions and integrated the react-hook-form for this component:)

walesch-yan · 2025-03-24T17:01:46Z

+                    <Alert
+                      className="mt-3"
+                      variant={feedback.variant}
+                      onClose={() => setFeedback({})}
+                      dismissible
+                    >
+                      {feedback.message}
+                    </Alert>


Added an alert as feedback for the user if the form was submitted correctly or if it failed

axelboc · 2025-03-25T08:13:31Z

+      setFeedback({
+        variant: 'success',
+        message: 'Feedback sent successfully',
+      });


The form state has an isSubmitSuccessful flag that tells you if the submit was successful (i.e. no error thrown in handleSubmit). And if sendFeedback throws an error, you can catch it and call the form's setError function as explained here: https://react-hook-form.com/docs/useform/seterror

This means that you don't need the feedback state and can just declare different alerts in the JSX based on isSubmitSuccessful and errors.

axelboc · 2025-03-25T08:16:55Z

+        variant: 'success',
+        message: 'Feedback sent successfully',
+      });
+      reset(DEFAULT_HELPFORM_VALUES);


This may reset isSubmitSuccessful and errors. I think there's an option that controls this: https://react-hook-form.com/docs/useform#resetOptions

walesch-yan · 2025-03-25T13:19:27Z

+  useEffect(() => {
+    if (isSubmitSuccessful) {
+      reset();
+      setShowSuccess(true);
    }
+  }, [isSubmitSuccessful, reset]);


According to the docs the recommended way of using reset is inside of useEffect using isSubmitSuccessful as trigger, I wrapped it inside the if, since reset will change the value of isSubmitSuccessful to false.

As one can see I also kept the useState here since there is no option to keep isSubmitSuccessful between resets but only isSubmitted (which becomes also true if the execution of submit fails). One could use resetField but it would go against the idea of sending multiple forms. Hence I decided to keep a local state variable.

Makes sense, in that case, I would move setShowSuccess(false) (currently in handleSubmit) into a separate use effect as well to hide the success message when the user starts filling in the form again:

useEffect(() => { if (isDirty) { // Hide success message if user starts filling in the form again setShowSuccess(false); } }, [isDirty]);

But it's probably overly complicating things... Forms suck.

Well in terms of UX your approach is probably better, thanks for all the suggestions!

Forms suck.

Couldn't agree more 😅

marcus-oscarsson · 2025-10-21T06:37:17Z

Hm, this one has been here for some time now. What would you like to do with it ?

walesch-yan commented Mar 19, 2025

View reviewed changes

Comment thread ui/src/containers/HelpContainer.jsx

walesch-yan commented Mar 19, 2025

View reviewed changes

Comment thread ui/src/containers/HelpContainer.jsx

walesch-yan commented Mar 19, 2025

View reviewed changes

walesch-yan force-pushed the yw-refactor-HelpContainer branch from 15e5587 to 6e1bf77 Compare March 19, 2025 12:51

axelboc approved these changes Mar 19, 2025

View reviewed changes

walesch-yan marked this pull request as draft March 19, 2025 14:16

walesch-yan force-pushed the yw-refactor-HelpContainer branch from 6e1bf77 to 60e34ce Compare March 24, 2025 16:59

walesch-yan commented Mar 24, 2025

View reviewed changes

axelboc approved these changes Mar 25, 2025

View reviewed changes

walesch-yan force-pushed the yw-refactor-HelpContainer branch from 60e34ce to 2f1f635 Compare March 25, 2025 13:08

walesch-yan commented Mar 25, 2025

View reviewed changes

walesch-yan mentioned this pull request Mar 25, 2025

fix typo in send_mail #1601

Merged

walesch-yan force-pushed the yw-refactor-HelpContainer branch 2 times, most recently from 2f02f43 to 88a95a0 Compare March 25, 2025 14:15

refactor HelpContainer

2802960

walesch-yan force-pushed the yw-refactor-HelpContainer branch from 88a95a0 to 2802960 Compare March 25, 2025 14:40

		const senderRef = React.useRef();
		const contentRef = React.useRef();

Uh oh!

Conversation

walesch-yan commented Mar 19, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-oscarsson commented Mar 19, 2025

Uh oh!

axelboc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axelboc Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walesch-yan commented Mar 19, 2025

Uh oh!

walesch-yan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walesch-yan Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axelboc Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-oscarsson commented Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

axelboc Mar 19, 2025 •

edited

Loading

walesch-yan Mar 25, 2025 •

edited

Loading

axelboc Mar 25, 2025 •

edited

Loading