Startingmodal #85

CalebLItalien · 2023-05-15T14:35:22Z

Starting modal rewrites.

Co-authored-by: Neil Daterao <ndaterao@gmail.com>

…etics preference

…starter-code Added a step to add a starting collection

CharlieMcVicker · 2023-05-15T14:43:26Z

closes #81

CharlieMcVicker

This is awesome and you have clearly learned so much!!

There are a couple things I'd love to see changed before we merge:

Names from copy/pasted code should be updated
We should look at useState calls and figure out if they are needed or are just creating the possibility for invalid state
I'm not convinced your "WHERE_FOUND" action actually makes it to the user state and updates the database. Also an action type like "SET_WHERE_FOUND" would be more in keeping with naming conventions.
The "exitWizard" and "exitAtEnd" functions are weird. I think that the steps/wizard abstraction is sort of borked at this point, and maybe we need to stop pretending that it is more general than it is.

src/components/GettingStartedModal/ChooseSetStep.tsx

CharlieMcVicker · 2023-05-21T18:31:19Z

src/components/GettingStartedModal/ChooseSetStep.tsx

+              onChange = {onRadioChanged}
+              checked = {collectionId == collection.id}
+            />
+            <label htmlFor={collection.id}>{collection.title} ({totalTerms(collection.sets)} terms) </label>


This is good for now, but I wonder if we should write short descriptions of the sets to put here instead of just # of terms.

CharlieMcVicker · 2023-05-21T18:32:03Z

src/components/GettingStartedModal/ChooseSetStep.tsx

+  goToNextStep,
+  goToPreviousStep,
+}: StepProps): ReactElement {
+  const [enabled, setEnbabled] = useState(collectionId != undefined); 


I think it's best practice to use !== here.

Maybe "canGoToNextStep" would be a better name for this. It's not clear what is enabled/disabled by this until the last line of the function. Also, is this state, or a function of state? Ie. should enabled always be equal to collectionId != undefined? Is there any time when we want to have the button enabled even though the collectionId isn't set? If it's actually a function of another field and not its own data, you could remove the "useState" call and just do:

const canGoToNextStep = collectionId !== undefined;

Yes this was a misunderstanding of state. I've changed this to use a global variable instead.

CharlieMcVicker · 2023-05-21T18:39:36Z

src/components/GettingStartedModal/GroupRegistrationStep.tsx

+        <fieldset>
+          <legend>Email Address</legend>
+          <input
+            type = "email"
+            name = "email"
+            value = {email}
+            onChange = {onEmailChanged}
+          />
+        </fieldset>
+        <fieldset>
+          <legend>Where did you find us?</legend>
+          <input
+            type = "whereFound"
+            name = "whereFound"
+            value = {whereFound}
+            onChange = {onWhereFoundChanged}
+          />
+        </fieldset>


So this is a weird HTML moment, but I don't think it's semantically correct to use fieldset and legend with a single input. The input is supposed to have a label that explains what it is, not a legend.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset

Maybe these two text inputs should be in a fieldset together and then each have their own label? That or they should be loose in the form or in divs.

This has been changed to make sense within the context of a fieldset.

CharlieMcVicker · 2023-05-21T18:40:53Z

src/components/GettingStartedModal/Preamble.tsx

+      <br></br>
+        - Filler text 1.
+        <br></br>
+        - Filler text 2.
+        <br></br>
+        - Filler text 3.
+      <br></br>


I know this is filler, but unordered lists should be written with the ul (Unordered List) and li (List item) tags, FWIW.

Also, this many <br/>s is scary -- maybe we need some components with padding/margins?

I temporarily changed it to the unordered list, do you want me to add a component for the code of conduct?

CharlieMcVicker · 2023-05-21T18:42:02Z

src/components/GettingStartedModal/StepIndicator.tsx

+/*
+ * Defines css for steps.
+ */
+export const StepIndicator = styled.h4`


Is this used by multiple files? If not, maybe it can just live in the file that uses it.

This has been moved to the only file that uses it, index.tsx.

CharlieMcVicker · 2023-05-21T18:44:38Z

src/components/GettingStartedModal/index.tsx

+      // TODO: perform any necessary cleanup or finalization
+    } else {
+      // if any required fields are missing, show an error message or prevent the user from exiting
+      alert("Please fill out all required fields.");


Is this code reachable? If not, why?

Form validation is a pain in the ass to do, but if we have this branch here that surfaces an error to the user, the user should have the information they need to solve the problem. If that's too much work, we need to report an error somehow.

Eg. If we are pretty sure this code never runs, leaving a line like the below would let future developers know that we never want to hit this branch.

throw new Error("Expected required fields to be filled out before attempting to exit");

Yes, this line can be reached and can be thrown to the user. I have added it to the exitWizardAtEnd function as well.

CharlieMcVicker · 2023-05-21T18:45:26Z

src/components/GettingStartedModal/index.tsx

+  function exitWizardAtEnd() {
+    // any part of wizard state could be undefined, so unpack it all
+    const { groupId, email, phoneticsPreference, collectionId, whereFound} = wizardState;
+    // add checks here to make sure fields are defined
+    if (groupId !== undefined && email !== undefined && phoneticsPreference !== undefined && collectionId != undefined && whereFound !== undefined) {
+      // reassemble state here (without Partial<>)
+      const fullState: WizardState = { groupId, email, phoneticsPreference, collectionId, whereFound};
+      // dispatch the actions for each step
+      steps.forEach((step) => step.commitState(fullState, userStateContext));
+    }
+  }


how is this different from exitWizard? When does each get called?

One exits upon filling out all required fields, the other ends after advanced features have been filled out. I have updated exitWizard to exitWizardNoAdvanced to make this more clear.

CharlieMcVicker · 2023-05-21T18:46:29Z

src/providers/UserStateProvider.tsx

  return (
    <userStateContext.Provider value={{ ...state, ...interactors, dispatch }}>
      {children}
      {(state.config.userEmail === null || state.config.groupId === null) && (


Should whereFound be on this list?

Maybe we should put this into a memo or something -- the condition is sort of complex to just shove inline like this (I know this is my code 👼)

I added the condition, could you help me with the memoization step? I'm confused on where to declare the memo of it all.

Maybe this should just be a constant defined earlier in the function.

Eg. add this earlier in the func near where you are doing hooks n stuff

const userNeedsSetup = (state.config.userEmail === null || state.config.groupId === null)

Then replace the inline condition with

(userNeedsSetup && (...Modal...))

I've made this change to the above suggestion. It might also be useful to define an array which defines necessary information from the user (ie, state.config.userEmail, state.config.groupId, etc.) and just looping threw those elements.

src/state/useUserState.ts

src/components/GettingStartedModal/GroupRegistrationStep.tsx

src/components/GettingStartedModal/PhoneticsStep.tsx

CharlieMcVicker

World's smallest most annoying nitpick

CharlieMcVicker · 2023-06-22T16:45:40Z

src/components/GettingStartedModal/ChooseSetStep.tsx

+  goToNextStep,
+  goToPreviousStep,
+}: StepProps): ReactElement {
+  var canGoToNextStep = collectionId !== undefined; 


I think this should be const -- var is evil and has terrible behavior!

I changed it to let so we'll be able to reassign it later.

CharlieMcVicker and others added 12 commits January 22, 2023 21:11

starter code for new onboarding workflow

0d53597

oop

34b38ac

oops pt2

9d2ea5f

Phonetics Preference Implementation (CherokeeLanguage#64)

b960c45

Co-authored-by: Neil Daterao <ndaterao@gmail.com>

keep state in wizard state; use radio buttons; implement default phon…

02760a7

…etics preference

Added a step to add a starting collection

a4465c7

edited formatting of intro steps

07596be

Merge pull request CherokeeLanguage#65 from fiabot/union-team/jan-23-…

e56a23f

…starter-code Added a step to add a starting collection

Merge branch 'main' into union-team/jan-23-starter-code

784b452

changes to the starting modal

617f18d

refactored getting started modal

3bb1237

pushing branch

b7d7bf7

CharlieMcVicker self-requested a review May 21, 2023 18:29

CharlieMcVicker requested changes May 21, 2023

View reviewed changes

fiabot reviewed May 22, 2023

View reviewed changes

src/components/GettingStartedModal/GroupRegistrationStep.tsx Outdated Show resolved Hide resolved

fiabot reviewed May 22, 2023

View reviewed changes

src/components/GettingStartedModal/PhoneticsStep.tsx Outdated Show resolved Hide resolved

CalebLItalien added 2 commits June 20, 2023 20:15

changes to starting modal. not all issues resolved, some to work out

1e2f3b7

Changes to getting started modal

b5cf5b7

CharlieMcVicker requested changes Jun 22, 2023

View reviewed changes

CalebLItalien added 2 commits June 23, 2023 07:37

small change

02e9d30

PWA

cd386e2

Startingmodal #85

Are you sure you want to change the base?

Startingmodal #85

Uh oh!

Conversation

CalebLItalien commented May 15, 2023

Uh oh!

CharlieMcVicker commented May 15, 2023

Uh oh!

CharlieMcVicker left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CharlieMcVicker 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants