Add labels in connect window #300
Conversation
xPaw
added
Type: Feature
|
Looks fine. Only thing is you appear to have added 2 aria labels, and no more. Is this deliberate? |
Yep. I added them because there's two fields (ip/port) but only one label, and adding aria labels describes the fields further. |
|
Ah right, makes sense. 👍 and merging |
| <div class="col-sm-9"> | ||
| <input class="input" name="name" value="<%= defaults.name %>"> | ||
| <input class="input" id="connect:name" name="name" value="<%= defaults.name %>"> |
There was a problem hiding this comment.
For future references, I think we should avoid using : in ids. I believe they need escaping if we want to grab them with jQuery or CSS, and in CSS it would look confusing as : is used for pseudo-elements.
I get the idea of namespacing though, and I think using - or _ for that works fine too.
There was a problem hiding this comment.
@maxpoulin64 used : in settings PR, so I went with same notation.
There was a problem hiding this comment.
I see. I still think we should avoid this for the 2 reasons given above.
There was a problem hiding this comment.
I agree with @astorije there. I totally didn't think at all about ever wanting to use those names in CSS selectors. I guess the only real option here is to use _ as everything else seems to be reserved (and - is used as a space replacement since we don't CamelCase CSS names). I wish there was a better looking character for representing namespaces that's not so close to - but apparently it's [a-zA-Z0-9_-]. Too bad, : made it easy to avoid id conflicts and looked good 😞 We could always escape it but it quickly gets ugly, especially in JS strings due to double escaping: $("#connect\\:name").
Add labels in connect window
Adds id/for in connect window so the labels are correctly linked.
Also makes port field a number type with min/max set.