-
Notifications
You must be signed in to change notification settings - Fork 25
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
Feature/dynamic preferences #1167
Conversation
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.
There are accessibility issues in these changes.
|
||
return ( | ||
<> | ||
<Select |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@dominikx96 how about /preferences/all? |
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.
👏 You fixed the issue(s)! Great work.
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.
👏 You fixed the issue(s)! Great work.
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.
👏 You fixed the issue(s)! Great work.
Deploy preview for clever-edison-cd22c1 ready! Built with commit 5dc694c https://deploy-preview-1167--clever-edison-cd22c1.netlify.app |
Thanks! I forgot to delete the old preference page from the next config, now netlify deploy is working! |
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.
👏 You fixed the issue(s)! Great work.
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.
👏 You fixed the issue(s)! Great work.
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.
👏 You fixed the issue(s)! Great work.
I also fixed tests to fetch listing with at least 2 preferences to test every step without fail :) |
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.
Nice work! Lots of moving parts here. I just had a few comments, mostly around making things easier to scan/maintain.
conductor.routeToNextOrReturnUrl() | ||
} | ||
|
||
const buildOptionName = useCallback((metaKey: string, option: string) => { |
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.
Can we take this out of a React callback and move it up somewhere as just a regular function? From what I can tell it doesn't require anything React-specific.
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.
Sure thing! I noticed, I can move it to the preference helper and share between public and partners forms :)
|
||
const watchPreferences = watch(allOptionFieldNames) | ||
|
||
function uncheckPreference(metaKey: string, options: FormMetadataOptions[]) { |
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.
use the const fnNane =
arrow syntax for consistency?
const hhMmembersOptions = useMemo(() => { | ||
const { applicant } = application | ||
|
||
const primaryApplicant = (() => { |
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'm finding it hard to follow here why an IIFE inside of a useMemo
is required? Can we simplify the pattern?
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 used IIFE because we have conditions and some logic inside, so it prevents the use of let
and I think it's more readable and it creates scope for primaryApplicant
.
/* | ||
Rehydration fix | ||
*/ | ||
const [hasMounted, setHasMounted] = useState(false) |
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.
Take a look at the OnClientSide hook—it's a way of doing this we've used before because of the weird form hydration issues
children: React.ReactChild | ||
} | ||
|
||
const ExpandableContent = ({ children }: ExpandableContentProps) => { |
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 realize this is sort of based on code I'd added in the previous preferences pages…but looking at this fresh I'm wondering if we can refactor to use a simple details/summary markup in the HTML directly. Prior art in the WhatToExpect component. Thoughts?
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.
Hmm, we can - but here we have two different labels: read less / read more.
We can create a listener to watch this state and show the proper label, but I'm not sure which one is simpler in this case :D
Also - this solution is open e.g for animation in the future 💅
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.
Good point. Maybe we can take a second look at this later on and see if it's something to standardize on for all the read more/less instances around the codebase.
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.
👏 You fixed the issue(s)! Great work.
Closes: #1130