feat(webapp): create generic email & password field#801
feat(webapp): create generic email & password field#801Rotzbua wants to merge 2 commits intodesec-io:mainfrom
Conversation
Rotzbua
changed the title
Rotzbua
force-pushed
the
webapp_input
branch
2 times, most recently
from
26eb0d1 to
675e5c5
Compare
Rotzbua
force-pushed
the
webapp_input
branch
from
675e5c5 to
e10ce28
Compare
peterthomassen
left a comment
peterthomassen
left a comment
There was a problem hiding this comment.
Besides some inline comments -- why go through the hassle of generalizing this?
I see there currently is some code duplication, but the functionality is not identical. For example, the change email form has a prepended letter icon, and for consistent spacing that requires other fields on that page to be prepended with a blank icon.
Making such functionality available by putting a bunch of knobs into a generalized implementation comes with a lot of complexity, and also code that's not needed so far (e.g. various pass-through props), which would be there instead of the current code duplication. Note that the new implementation adds about 65% more LOC than it removes. I'm not yet convinced that's a good idea.
| validate-on-blur | ||
| <generic-email | ||
| v-model="email" | ||
| label="Current Email Address" |
There was a problem hiding this comment.
Isn't this overwritten by computed?
| const icon = this.readonly ? mdiEmailLock : mdiEmail; | ||
| const ruleDefs = { | ||
| required: v => !!v || 'Email is required.', | ||
| valid: v => (v !== undefined && !!email_pattern.test(v)) || 'We need an valid email address for account recovery and technical support.', |
There was a problem hiding this comment.
This text either should be generic or configurable through a prop.
| ErrorAlert, | ||
| }, | ||
| data: () => ({ | ||
| user: useUserStore(), |
|
Thanks for the feedback 👍 I will have a look. |
Rotzbua
force-pushed
the
webapp_input
branch
from
a5343ed to
e5b3823
Compare
Are you interested in finishing this, @Rotzbua? |
2 participants
Change
Reference
Mentioned in #752