-
Notifications
You must be signed in to change notification settings - Fork 26
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
1721/remove business logic from HouseholdMemberForm component #1722
Conversation
✔️ Deploy Preview for dev-bloom ready! 🔨 Explore the source changes: 186d0ff 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/613a2f5b1a71030008bac580 😎 Browse the preview: https://deploy-preview-1722--dev-bloom.netlify.app |
✔️ Deploy Preview for dev-partners-bloom ready! 🔨 Explore the source changes: 186d0ff 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/613a2f5b890d0a0008411625 😎 Browse the preview: https://deploy-preview-1722--dev-partners-bloom.netlify.app |
✔️ Deploy Preview for dev-storybook-bloom ready! 🔨 Explore the source changes: 186d0ff 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/613a2f5bcf361900086162ea 😎 Browse the preview: https://deploy-preview-1722--dev-storybook-bloom.netlify.app |
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.
@emilyjablonski I've noticed a problem with editing members. I think it's not related to your changes, but definitely, it's a problem :D
Steps to reproduce:
http://localhost:3000/applications/household/member?memberId=1#
- create 2 additional HH members (primary + 2 others)
- delete one HH member (not primary)
- try to edit other HH member (not primary)
It occurs empty edit member form.
If the problem that @dominikx96 calls out isn't a quick fix that makes sense to do here, can you please open a separate bug and add it to the backlog. |
Good catch @dominikx96! The problem exists on dev, and to repro the person you delete needs to be the first person you added (so the third person has the index shifted). I pushed a fix. |
28bd956
to
18f1d8f
Compare
✔️ Deploy Preview for clever-edison-cd22c1 ready! 🔨 Explore the source changes: 28bd956 🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/61294389628a31000788625b 😎 Browse the preview: https://deploy-preview-1722--clever-edison-cd22c1.netlify.app |
✔️ Deploy Preview for clever-edison-cd22c1 ready! 🔨 Explore the source changes: 9e804c1 🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/612fb122bacbd70007d16312 😎 Browse the preview: https://deploy-preview-1722--clever-edison-cd22c1.netlify.app |
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 wonder if this should even remain in ui-components
or just be relocated to the public site. Is it all that reusable outside of its original scenario?
{editMode ? ( | ||
<a id="edit-member" className="edit-link" href="#" onClick={editMember}> | ||
<a |
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.
While you're in here…this should probably be changed to button
and not be an a
since it doesn't actually go to a real href.
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.
updated!
9e804c1
to
186d0ff
Compare
…ld-member-form
Pull Request Template
Issue
Addresses #1721
Description
Removes business logic from HouseholdMemberForm component.
Type of change
How Can This Be Tested/Reviewed?
Start an application. At the household members part, add some household members, and click the edit button to ensure the routing works properly to each individual person.
Checklist: