[Initial review] Create new rule from CSS quick edit#5532
Conversation
…les. Mostly working, but header doesn't show in that case.
…tags to just use normal jQuery way.
…e/brackets into css-quick-edit-new-selector
* Factor out wrapper, header, editor holder, and filename divs so they're permanent instead of recreating them on every switch * Change `editors` field to just `editor` since we only ever have one editor * Make appearance of message area a little closer to final design
…e/brackets into css-quick-edit-new-selector
…e/brackets into css-quick-edit-new-selector
…uick-edit-new-selector
…e/brackets into css-quick-edit-new-selector
ghost
assigned
There was a problem hiding this comment.
nit: the other functions here have jsdoc comments. I'm not actually very particular about jsdoc comments for simple interior functions like this, but it would be more consistent to have a comment since the others do.
There was a problem hiding this comment.
@redmunds - do you want to add comments for those functions? If not I can take a stab at it.
|
It looks like a bad CM submodule SHA snuck in again. @larz0, remember to do a |
|
I'm done with my changes. @njx Let me know if there's anything I missed. |
There was a problem hiding this comment.
I'll grant that this API likely won't have a lot of consumers, but it seems like any consumer of this API would want this performance improvement, unless there are likely different approaches that would be used to hide and show the editor or different desired timings...
I don't know how others would use the createInlineEditorForDocument API, but it does seem likely that someone would call it without hiding the container first.
There was a problem hiding this comment.
That's a good point, though I do think there really aren't any other likely callers of that API. But given that, it seems reasonable to move the hide/show into it.
|
I'll finish the review in the morning (Eastern time). |
|
I think I've addressed all the comments so far. |
There was a problem hiding this comment.
nit: for consistency with the changes below, it might be nice to change this to $("<li/>")
|
Review complete. Looks good, and I really like the new feature! Only some minor stuff here. |
|
One thing I didn't notice while reviewing is a keyboard shortcut for adding a new rule, which is mentioned in the criteria. Did I just miss it somewhere? |
|
No. Any suggestions on which keyboard shortcut to use? |
|
cmd-shift-N? We could also override existing shortcuts since this is a contextual shortcut. |
|
Also, we'd probably want the shortcut to be displayed on a tooltip for the button, given that it won't be on a menu. |
|
Hmmm. This is a little fuzzy to me, because we put the "explicit keyboard shortcut to create new rule" (from the main editor) in the Out of Scope list. I don't exactly recall talking about having a specific (separate) shortcut that would work from within the inline editor...though I think we did talk about whether there was a way to make the button take focus from the keyboard (so you could hit Return to activate it). I'll bring it up in standup today. |
|
Fixed the remaining code cleanup issues. I'm tracking the keyboard shortcut as a separate task, so we should be able to merge this now unless you see anything else. |
|
BTW, one other little tweak I want to make is to make it so that if there's only one stylesheet in your project, clicking New Rule immediately creates the rule in that stylesheet instead of popping up the dropdown. I'll add that in a future pull request. |
|
Looks good. Merging. |
[Initial review] Create new rule from CSS quick edit
4 participants
To @dangoor - let me know if you want a quick walkthrough of the changes by phone. Also note that the functionality to update the rule list if the user edits a selector hasn't been added yet, but everything else should work.