-
Notifications
You must be signed in to change notification settings - Fork 20
fix link card dialog #7680
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
base: yuri/link-visa-card-with-registration-front-end
Are you sure you want to change the base?
fix link card dialog #7680
Conversation
08b0169 to
c4d7109
Compare
elwinschmitz
left a comment
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 is some overlap with the review of:
#7655 (review)
...registration-debit-cards/components/link-card-on-site-dialog/link-card-dialog.component.html
Show resolved
Hide resolved
...registration-debit-cards/components/link-card-on-site-dialog/link-card-dialog.component.html
Show resolved
Hide resolved
...registration-debit-cards/components/link-card-on-site-dialog/link-card-dialog.component.html
Show resolved
Hide resolved
...registration-debit-cards/components/link-card-on-site-dialog/link-card-dialog.component.html
Show resolved
Hide resolved
| tokenCodeToClean, | ||
| }: { | ||
| tokenCodeToClean: string; | ||
| }) => computed(() => tokenCodeToClean.replaceAll('-', '')); |
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.
This is 'just a pure function', right?
Does it need to be computed at all?
I don't think we need a lot of that Signal-magic-dust here...
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.
Also; I think we can even check for a bigger range of "weird characters people will use";
So we could "remove everything that is not a digit", by:
| }) => computed(() => tokenCodeToClean.replaceAll('-', '')); | |
| }) => computed(() => tokenCodeToClean.replace(/\D/g, ''); |
So it would catch (double) spaces, dashes, underscored, periods, comma's, etc...
| [modal]="true" | ||
| [closable]="true" | ||
| [dismissableMask]="true" | ||
| [style]="{ width: '45rem' }" |
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.
Would there be a safety-value for the min-width? Or a valid max-width?
That way, this dialog would became a lot more "safe for responsive use"...
AB#39408
Describe your changes
Checklist before requesting a code review
Portal preview-deployment
https://happy-rock-0411d2003-7680.westeurope.3.azurestaticapps.net