-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(forms): Auto ID generation client side if no id prop provided #882
Conversation
lib/mixins/id.js
Outdated
safeIdPlus(str) { | ||
str = String(str || '').replace(/\s+/g, '_'); | ||
return (this.safeId && Boolean(str)) ? `${this.safeId}_${str}` : null; | ||
} | ||
} | ||
}; |
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.
IMHO will be more flexible to have one universal method at this API, which accepts two optional params: generateId(String suffix = '', Boolean isPrefix=false)
It could be something like that:
generateId(str='', isPrefix=false) {
str = String(str).replace(/\s+/g, '_');
if(!!str && this.safeId){
return isPrefix? `${str}_${this.safeId}`: `${this.safeId}_${str}`
}else if(this.safeId){
return this.safeId
}
return null
}
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 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.
True, but in most cases, this is an internal ID's will have suffix only.
Originally I had planned on a single method, which appended the suffix if provided (similar to what you have above), but I wanted wanted the suffixed ID to return null if the suffix was invalid (i.e. a boolean, undefined, etc).
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.
but I could convert safeId
to a method, and optionally append the first argument
lib/mixins/id.js
Outdated
if (!id) { | ||
return null; | ||
} | ||
suffix = String(suffix || '').replace(/\s+/g, '_'); |
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.
With default value at function definition you do not need to do suffix || ''
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.
on the whole, this PR now looks better with one flexible method 👍
Codecov Report
@@ Coverage Diff @@
## 1.x #882 +/- ##
=========================================
+ Coverage 40.5% 40.89% +0.38%
=========================================
Files 88 88
Lines 2244 2252 +8
Branches 645 647 +2
=========================================
+ Hits 909 921 +12
+ Misses 1177 1175 -2
+ Partials 158 156 -2
Continue to review full report at Codecov.
|
This is a client side only ID generator, which supplies an ID attribute on form control when no ID prop is provided.
ID generation only runs on client side, during the mounted() hook (after SSR rehydration, if applicable).
The presence if IDs, even if not in the initial SSR rendered HTML will greatly improve accessibility.
Other components that can take advantage of IDs (i.e. dropdowns, carousel, etc) can use the new
id.js
mixin