-
Notifications
You must be signed in to change notification settings - Fork 3
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
1430: Add query params to card self service form #1749
Conversation
Some general ideas to your pr. You can just add a fkt if you add Nullish Coalescing in the handle query params fkt and return undefined you can even avoid null checking while initializing the card const fullName = cardQueryParams.get('name') ?? undefined Example const [cardQueryParams] = useSearchParams()
const {fullName, birthday, koblenzReferenceNumber} = handleQueryParams(cardQueryParams)
const [selfServiceCard, setSelfServiceCard] = useState(
initializeCard(projectConfig.card, undefined, { fullName, expirationDate: null, extensions: {birthday, koblenzReferenceNumber } })
)
) After generating a card successfully the queryParams should be removed. You can get a set fkt from the setSearchParams(undefined, { replace: true }) |
@f1sh1918 thanks for the feedback, I applied your suggestions, pls check if I understood you right. |
administration/src/bp-modules/self-service/hooks/useCardGeneratorSelfService.ts
Outdated
Show resolved
Hide resolved
3cf521b
to
0922e1b
Compare
0922e1b
to
6224bbb
Compare
administration/src/bp-modules/self-service/hooks/useCardGeneratorSelfService.ts
Outdated
Show resolved
Hide resolved
administration/src/bp-modules/self-service/hooks/useCardGeneratorSelfService.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # administration/src/bp-modules/self-service/CardSelfServiceForm.tsx # administration/src/bp-modules/self-service/hooks/useCardGeneratorSelfService.tsx
1eb8192
to
ee12261
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 think something is not quite right yet, when I open the URL as you suggested nothing happens except the query params being removed. None of the supplied properties is used to initialize the card.
const [selfServiceCard, setSelfServiceCard] = useState( | ||
initializeCard(projectConfig.card, undefined, { expirationDate: null }) | ||
) | ||
const [cardQueryParams, setSearchParams] = useSearchParams() |
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.
🔧
const [cardQueryParams, setSearchParams] = useSearchParams() | |
const [searchParams, setSearchParams] = useSearchParams() |
const [isLoading, setIsLoading] = useState<boolean>(false) | ||
const [selfServiceState, setSelfServiceState] = useState<CardSelfServiceStep>(CardSelfServiceStep.form) | ||
const [deepLink, setDeepLink] = useState<string>('') | ||
const [code, setCode] = useState<CreateCardsResult | null>(null) | ||
|
||
useEffect(() => { | ||
setSearchParams(undefined, { replace: true }) |
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 perhaps move this inside the initializing method to avoid the extra use effect and possible side effects?
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 think its better to move this to the createKoblenzPass
function to only remove them if the card was created successfully, so the params will not be removed if there is a page refresh (maybe by accident)
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.
@f1sh1918 pls check if that's what you mean
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 think you can just reuse the hook in CardSelfServiceForm
@seluianova
const [_, setSearchParams] = useSearchParams()
and execute the function as you did. Then you don't have to pass down the function neither to CardSelfServiceForm
nor adjust the useCardSelfService
hook
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.
hopefully, it's good to go now, thx 🙈
Ah, sorry, I forgot to update the PR description. Param names should be the same as in card config now. |
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.
Tested on firefox, works with the correct link with correctly set query params. I updated the PR description.
Works as expected that way :)
const [cardQueryParams, setSearchParams] = useSearchParams() | ||
const [selfServiceCard, setSelfServiceCard] = useState(() => { | ||
const headers = getHeaders(projectConfig) | ||
const values = headers.map(header => cardQueryParams.get(header)) | ||
return initializeCardFromCSV(projectConfig.card, values, headers, undefined, true) | ||
}) |
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 really like your implementation. If you feel motivated, feel free to add the same functionality to the useCardGenerator
hook and remove the old implementation from AddCardsFrom
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 think we should merge this soon to create the next beta artifact and i would prefer to implement this in a separate task
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 have created a new ticket for that #1799
@steffenkleinle could you please check and adjust the description (if needed), because I'm not sure I could describe this task nicely, and wouldn't want to mislead with the description.
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.
Nicely done 👍
One small comment should be resolved before merging
const [cardQueryParams, setSearchParams] = useSearchParams() | ||
const [selfServiceCard, setSelfServiceCard] = useState(() => { | ||
const headers = getHeaders(projectConfig) | ||
const values = headers.map(header => cardQueryParams.get(header)) | ||
return initializeCardFromCSV(projectConfig.card, values, headers, undefined, true) | ||
}) |
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 think we should merge this soon to create the next beta artifact and i would prefer to implement this in a separate task
const [isLoading, setIsLoading] = useState<boolean>(false) | ||
const [selfServiceState, setSelfServiceState] = useState<CardSelfServiceStep>(CardSelfServiceStep.form) | ||
const [deepLink, setDeepLink] = useState<string>('') | ||
const [code, setCode] = useState<CreateCardsResult | null>(null) | ||
|
||
useEffect(() => { | ||
setSearchParams(undefined, { replace: true }) |
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 think its better to move this to the createKoblenzPass
function to only remove them if the card was created successfully, so the params will not be removed if there is a page refresh (maybe by accident)
78babd9
to
d884657
Compare
d884657
to
0dbdae9
Compare
* 1430: Add query params to card self service form
Short description
When the user taps the “Extend Card” button in the app, the self-service portal should open with the pre-filled data.
In this PR, I added support for query parameters in the /erstellen route.
I feel a bit lost in React, so perhaps my implementation is not optimal, pls take a look.
Proposed changes
Side effects
no?
Testing
Open url with query params
http://localhost:3000/erstellen?name=Karla%20Koblenz&ref=123K&geburtsdatum=2003-06-10
localhost:3000/erstellen?Name=Karla%20Koblenz&Referenznummer=123K&Geburtsdatum=10.06.2003
The form should be pre-filled with the provided data
Resolved issues
Part of: #1430