Aria describedby tooltip 13480 #13557
Conversation
|
I would advise not including the |
|
|
||
| Tooltip.prototype.setAriaDescribedBy = function() { | ||
| var tipId = this.uid(this.type) | ||
| this.$tip.attr('id',tipId ) |
There was a problem hiding this comment.
please ensure that there's a space after all commas in your code
|
@cvrebert cleaned up the code from your feedback, thanks. I can squash the commits down if we need to as well. |
| Tooltip.prototype.uid = function (prefix) { | ||
| var id = prefix + Math.floor( Math.random() * 1000000 ) | ||
|
|
||
| while ($('#' + id).length) { |
There was a problem hiding this comment.
Perhaps we should use document.getElementById here for speed? I'll leave that for @fat to decide.
There was a problem hiding this comment.
I think that makes sense, we don't too slow down much from adding more DOM queries.
There was a problem hiding this comment.
i dont think we'd get much speed value given sizzle's code paths, but the code would probably look cleaner
There was a problem hiding this comment.
Tooltip.prototype.uid = function (id) {
do (id += Math.floor(Math.random() * 1000000)); while (document.getElementById(id))
return id
}
|
Yes, please squash if you're comfortable doing so. |
|
How does that look now, I am a little puzzle why it is including your two commits @cvrebert, if you don't mind fixing the history as it should I would be thankful. |
|
im a little torn because i'd like to see a global accessibility plugin (something like we have for animation), which handles things like uid generation (which will likely be needed by other plugins as we continue work on accessibility). That said, if we're only using it here, it's probably fine for now |
|
|
||
| Tooltip.prototype.setAriaDescribedBy = function() { | ||
| var tipId = this.$tip.attr('id', this.uid(this.type)) | ||
| this.$element.attr('aria-describedby', tipId) |
There was a problem hiding this comment.
this.$element.attr('aria-describedby', this.$tip.attr('id', this.uid(this.type)))
There was a problem hiding this comment.
this.$element.attr('aria-describedby', this.$tip.attr('id', this.uid(this.type)))
I tried using this at first, but it would return the DOM object in the ``... this.$tip.attr('id', this.uid(this.type)))`
|
All good feedback @fat, I will make those changes and resubmit in a moment. Regarding making the unique id function global, that could be helpful in the future. Being able to relate elements using aria-[whatever] is pretty common, so it might be helpful later. |
| @@ -147,8 +147,13 @@ | |||
| var that = this; | |||
|
|
|||
| var $tip = this.tip() | |||
| var tipId = this.type | |||
|
|
|||
| do (tipId += Math.floor(Math.random() * 1000000)); while (document.getElementById(tipId)) | |||
There was a problem hiding this comment.
Not that it would really matter, but
do tipId += ~~(Math.random() * 1000000)
while (document.getElementById(tipId))does the same, requires no semicolon and double bitwise NOT performs slightly better than Math.floor. Also, it is shorter in the minified JS.
|
Here we go, hopefully I got it this time. I think I integrated all the feedback. The only think I am wondering about is if the test coverage is good enough? |
|
The concatenated JS is outdated, probably would be best to remove it completely from the PR. |
|
I'll be happy to properly rebase this when merging it. Just need @fat to approve the final draft. |
Generates a unique id for tooltip and adds [aria-describedby] to the element it is called on. Resolves issue twbs#13480 - set up test - linted the code styles - passed the tests - integrated feedback from @cvrebert
|
changes lgtm |
|
Sorry What is lgtm? |
|
"looks good to me" |
|
Now running Sauce tests: https://travis-ci.org/twbs/bootstrap/jobs/25905569 |
|
Sauce test passed. Merging per @fat's LGTM. |
Aria describedby tooltip: #13480
Pull request for issue #13480