-
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
1419: Create card form #1648
1419: Create card form #1648
Conversation
…, add error handling, override isValid() for SelfServiceCard to allow undefined expirationDate
@@ -60,6 +61,7 @@ const Router = () => { | |||
{ path: '/antrag-einsehen/:accessKey', element: <ApplicationApplicantController /> }, | |||
] | |||
: []), | |||
...(projectConfig.selfServiceEnabled ? [{ path: '/erstellen', element: <CardSelfServiceView /> }] : []), |
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.
maybe there are some ideas for a better route name
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.
We currently have a mix of english and german path names 🙄
Just a general though: Maybe it would make sense to have an project id or project abbreviation in paths? so it is for us also clear this "erstellen" path only applies to koblenz. e.g. /koblenz/erstellen or /ko/erstellen or /4/erstellen.
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.
The issue here is that we then have on production
koblenz.sozialpass.app/koblenz/erstellen
which makes no sense
the projectId
can not be hardcoded in the projectConfig since it is created by the backend, but would have to be retrieved from the api which is difficult because we need to call the api before executing the router.
But you are right, we might have to discuss this
2e778d7
to
fb46a4f
Compare
Just one thing i find: If you have a birthday on the default date and do not touch the date picker it says that the data is not valid. So it should either have no default or accept that the default is a valid input. But this is minor as not many people will have birhtday on this exact date |
Thx for the hint. Unfortunately the styling of the mui textfield placeholders is really horrible since they use this placeholder label mechanism which we currently don't use Maybe its a ok compromise to set an initial date like this and use placeholder color (in fact the state is still null so behind the scenes this value is not really set for a card) |
…d test for windows dimension hook, fix margin of tooltip, add todos
fb46a4f
to
7ae5d82
Compare
# Conflicts: # administration/src/bp-modules/cards/AddCardForm.tsx # administration/src/cards/PdfFactory.ts # administration/src/cards/extensions/BirthdayExtension.tsx # administration/src/project-configs/koblenz/dataPrivacyBase.tsx
cfb7dd0
to
fd2a124
Compare
fd2a124
to
1ef5a0f
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.
If entering wrong data i get the response "Server nicht erreichbar". This is the wrong error message. It is related to this issue: #1660
I do not like the error message about missing data being initaliy displayed and i think this is not state-of-the-art, most websites only show errors once you try submitting. Also the red error border is not nice, when initically opening the site.
The form is jumping when scrolling down completely and then entering the data. (see attached video)
1419.mp4
can you please add the example csv to the code base (administration/resources)
I was not able to create a card. I did import the csv successfully, but the correct data led to an error 404 Not Found- no body was set
Did not fully review, as i cannot proceed with testing properly because of this error.
@@ -60,6 +61,7 @@ const Router = () => { | |||
{ path: '/antrag-einsehen/:accessKey', element: <ApplicationApplicantController /> }, | |||
] | |||
: []), | |||
...(projectConfig.selfServiceEnabled ? [{ path: '/erstellen', element: <CardSelfServiceView /> }] : []), |
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.
We currently have a mix of english and german path names 🙄
Just a general though: Maybe it would make sense to have an project id or project abbreviation in paths? so it is for us also clear this "erstellen" path only applies to koblenz. e.g. /koblenz/erstellen or /ko/erstellen or /4/erstellen.
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 also added this file to frontend assets. Do we add our assets duplicated on adminstration and frontend. Does this make sense? 🤔
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.
we have currently no mechanism to exchange assets between frontend and administration. I can not directly import assets from frontend. In general one source would make sense for sure
administration/src/bp-modules/self-service/CardSelfServiceActivation.tsx
Outdated
Show resolved
Hide resolved
administration/src/bp-modules/self-service/CardSelfServiceForm.tsx
Outdated
Show resolved
Hide resolved
administration/src/bp-modules/self-service/CardSelfServiceForm.tsx
Outdated
Show resolved
Hide resolved
generateCards: () => Promise<void> | ||
} | ||
|
||
const getTooltipMessage = (cardsValid: boolean, dataPrivacyAccepted: boolean): 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.
I think the handling of errors is quite inconsistent:
only the checkbox-error/missing Datenschutzerklärung is displayed on the bottom, the other things are only displayed by a red border, which i think is not enough. There should be a message why this data is not correct.
The error message should only be displayed after "Pass erstellen" was clicked.
i know the design is a bit inconsistent here, too
also the design is very different from the flow we have in the application form, there we have the button enabled and then show errors once the button is clicked, which i think is state-of-the-art. In the design on the initial view no error messages are displayed, which i think should be the case, but of course cannot be realized with the disabled button.
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 already created an issue that we need sth like an untouched state for this
#1664
rightElement={ | ||
<ClearInputButton viewportSmall={viewportSmall} onClick={clearNameInput} input={card.fullName} /> | ||
} | ||
intent={card.isFullNameValid() ? undefined : Intent.DANGER} |
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.
Name input should only be valid if first and lastname is entered and both is at least 2 or 3 chars long. Otherwise people may only input their lastname and share the pass with relatives.
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.
added this task also to #1670 since we need then a hint for that
For me the "server nicht erreichbar" issue doesn't appear. If i'm entering wrong data i get a correct error message. Even this error message should be shown in the alert box not as a toast. Will adjust that. For routing names we have the issue that we use german names for the routes of end user of the card and english for the service users. I just continued this. Since most of the links are used somewhere like Freinet or Care4 or linked by cities, its not that easy to stick to a general conventions. we could create redirects for these routes and keep everything in english. Not sure what is the best solution |
administration/src/cards/extensions/components/ClearInputButton.tsx
Outdated
Show resolved
Hide resolved
administration/src/bp-modules/self-service/CardSelfServiceActivation.tsx
Outdated
Show resolved
Hide resolved
administration/src/bp-modules/self-service/CardSelfServiceView.tsx
Outdated
Show resolved
Hide resolved
I think it would be cool if there wouldn't be too many/big changes here anymore and further improvement could be done in separate PRs/issues instead due to my refactoring in #1667. |
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.
administration/src/bp-modules/self-service/CardSelfServiceForm.tsx
Outdated
Show resolved
Hide resolved
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.
very much minor and probably optional, but:
when a user opens a form, they immediately see validation errors, but they haven't touched anything yet.
I think ideally the validation should be run after the user enters something.
There is already an open issue for implementing an untouched state. |
Yes I created couple of issues for improvement. |
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.
Works as expected if you actually follow the testing instructions 🤦
@ztefanie can you please recheck or at least remove your changes requested to unblock this pr |
# Conflicts: # administration/src/project-configs/koblenz/dataPrivacyBase.tsx
Outdated and blocking merging
Short description
As a user i want to create and activate my koblenz pass
Proposed changes
Side effects
TODOS
Testdata
Testdata with base information for testing
Testing (mobile mode)
selfServiceEnabled: true
is set.curl -F file=@import_testdata_local.csv http://0.0.0.0:8000/users/import
Resolved issues
Fixes: #1419
Fixes: #1420