-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat(webapp): create generic email & password field #801
base: main
Are you sure you want to change the base?
Conversation
26eb0d1
to
675e5c5
Compare
675e5c5
to
e10ce28
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text either should be generic or configurable through a prop.
ErrorAlert, | ||
}, | ||
data: () => ({ | ||
user: useUserStore(), |
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.
What is this needed for?
Thanks for the feedback 👍 I will have a look. |
a5343ed
to
e5b3823
Compare
Change
Reference
Mentioned in #752