Dialog: Inline code review #795
Conversation
ui/jquery.ui.dialog.js Outdated
There was a problem hiding this comment.
attroperties... Probably not needed anymore since jQuery 1.6 understands the difference.
There was a problem hiding this comment.
Yeah, since I saw the tooltip issue and fix, we can probably drop this. I'll test it.
There was a problem hiding this comment.
Dropping this causes tests to fail that expect an empty string as the default for the title option - without the typeof check here, the option ends up being reset to null instead. Arguably the right default value though.
| Regarding If not enough, we can look into just removing background-images in affected IE versions, like we removed the background for tooltip's in IE6 (f83f07d) |
| @jzaefferer, I avoided rebasing here so you don't lose your place in history. Rebase at will if you'd like. |
| I just rebased this and the dialog branch. |
There was a problem hiding this comment.
No tests for when focus shouldn't happen?
…fault null, as it should be. No back-compat, as the behaviour doesn't change.
…date above that to access old option value.
… refactoring _setOption, no need for switch.
…tch (no fallthroughs).
…d for the uiDialog closure. Use this.uiDialog and remove the variable.
… instead. Wasn't needed anymore (previous refactorings).
…two keydown handlers into one.
…emoved outdated TODOs.
| Addressed all of the above, and updated the dialog API buttons example to also use the array syntax: jquery/api.jqueryui.com@561c562 |
3 participants
@scottgonzalez can you check out my code comments here? Not intended to actually land anywhere, just to get a grip on the code. Also was the convenient thing to do on the plane.
This includes commits from #794 and targets that PR branch, so just the review commit shows up here.