Dialog: Add option to put the dialog title in a header element #2275
Conversation
|
Ok i've finished an initial draft. I am not sure if the multiline syntax and formatting i've used is correct but i had to satisfy the linter and get to a line length < 100 characters. and i've also added a test. i've created a modal dialog and set the desired tag name to h2. i've picked an html tag != the default value |
|
The good thing, the test works (the expected value is an h2 but the actual value is still a span). so my fix for the line length linting error isn't working (as a one liner it works locally). |
|
Got the test to green with the help of @rocketeerbkw. the only thing we were not sure if it was necessary to add the new option to the common-deprecated.js file as well. |
ui/widgets/dialog.js
Outdated
| @@ -437,7 +438,8 @@ $.widget( "ui.dialog", { | |||
| } | |||
| } ); | |||
|
|
|||
| uiDialogTitle = $( "<span>" ).uniqueId().prependTo( this.uiDialogTitlebar ); | |||
| uiDialogTitle = $( this.options.uiDialogTitleTagName ) | |||
There was a problem hiding this comment.
Passing user input directly to the jQuery factory is dangerous - apart from obvious abuse when people pass selectors instead of tags here, they can e.g. pass <script>, etc. Protecting from that would require significantly more code, so I'm thinking we should actually change the approach.
How about introducing an option called uiDialogTitleHeadingLevel with the default of 0 and possible values all integers between 0 & 6; you can use Number.isInteger for that. We can check the value and revert to 0 if it's invalid. Then, for 0 we use <span> but for other values we use a proper heading level.
That should cover the important use cases here and avoid possible security issues. Thoughts?
There was a problem hiding this comment.
in my initial draft i kept the <> out of the option but thought it would over complicate things so i ended up with the current version. but i agree that your suggested approach would be the better choice. would something like that work?
function between ( headingLvl ) {
return Number.isInteger( headingLvl ) && headingLvl > 0 && headingLvl <= 6 ? "h" + headingLvl : "span";
};
uiDialogTitle = $( "<" + between( this.options.uiDialogTitleHeadingLevel ) + ">" ).uniqueId().prependTo( this.uiDialogTitlebar );
i am not sure if it would be a clean code wise and i also dont know what the preferred name for the function would be. but the shown snippet is at least already working: the only difference to your suggestion is that the condition is only true between 1 and 6 everything else 0 included becomes a span.
and i would also add a few test cases.
There was a problem hiding this comment.
That works, but since this logic is quite simple and only used in a single place, it's not necessary to create a function for it, I think. You can just define a local variable with the computed tag name & use it below.
Try to keep lines ideally below 80 characters (and definitely below 100), breaking them into multiple lines when necessary.
And yes, a few test cases would be needed, when the option was:
- not passed (testing default behavior).
0167(testing that it reverts tospan)"foo"(testing that it reverts tospan)
Those should be enough, I think. Optionally, instead of testing 1 & 5 individually, you can create a loop through values 1-6, but I don't think it's absolutely necessary.
There was a problem hiding this comment.
ok thanks! the suggested changes are in place and the tests are green.
There was a problem hiding this comment.
LGTM. I'd like one addtional review but we're not in a hurry - 1.14.0 was released today so the next release won't happen immediately.
We also need docs for this new option before we can include it. The docs for dialog are at https://api.jqueryui.com/dialog/. Can you document it? It'd be a similar entry to closeOnEscape here:
https://github.com/jquery/api.jqueryui.com/blob/1c46b8ea6ca5b0220856649283c049c4946cb391/entries/dialog.xml#L134-L137
We also need a note that 1.13.1 is the first release with this API, like the one we have here: https://api.jqueryui.com/datepicker/#option-onUpdateDatepicker. This is triggered by adding the <added> tag; see https://github.com/jquery/api.jqueryui.com/blob/1c46b8ea6ca5b0220856649283c049c4946cb391/entries/datepicker.xml#L461-L465 for the source of it.
|
so you mean creating a pull request for api.jqueryui..com including the note about the initial release in 1.14.1? i can create that initial draft over there. And do i need to create an issue in the api.jqueryui.com issue queue first or would it be ok to directly open a pull request there? cuz on the jqueryui repo ive also had to use the issue number of the issue for the branch. for this issue here the branch is called for example 2271-add-option-uidialogtitle-html-element. |
|
Just a PR will be fine. We can have it reference the corresponding UI issue + PR in the commit message. |
|
oki, and should i omit using an issue number in the branch name? |
|
The branch name doesn’t matter, you can use whatever is convenient for you. |
|
@fnagel can you review as well? |
|
Thanks, merged! The change will arrive in jQuery UI 1.14.1. |
|
Thank you! and one question do you already have a rough plan when you want to release 1.14.1? i ask if there is a slight chance it might be released before the first beta for the next minor release drupal (11.1) arrives , which is planned for november 11th? |
|
@rpkoller yeah, a release in October is likely if nothing critical comes out and the milestone (https://github.com/jquery/jquery-ui/milestone/8) is clean; currently, there's one issue to fix there, but it's simple. |
|
that would be perfect, thank you for the headsup! |
|
@rpkoller jQuery UI 1.14.1 with this change has been released. |
PR for #2271. Initial draft adding an option based on the naming @mgol suggested on the issue (
uiDialogTitleTagName) and usingspanas the default. Currently working on adding tests as well in the next step.