Skip to content
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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Rotzbua
Copy link
Contributor

@Rotzbua Rotzbua commented Aug 31, 2023

Change

  • generic email field
  • generic password field

Reference

Mentioned in #752

@Rotzbua Rotzbua changed the title Webapp input feat(webapp): create generic email & password field Aug 31, 2023
@Rotzbua Rotzbua force-pushed the webapp_input branch 2 times, most recently from 26eb0d1 to 675e5c5 Compare August 31, 2023 21:59
Copy link
Member

@peterthomassen peterthomassen left a 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"
Copy link
Member

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.',
Copy link
Member

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(),
Copy link
Member

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?

@Rotzbua
Copy link
Contributor Author

Rotzbua commented Dec 26, 2023

Thanks for the feedback 👍 I will have a look.

@Rotzbua Rotzbua marked this pull request as draft December 26, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants