search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] [Initial review] Create new rule from CSS quick edit #5075

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by njx Wednesday Oct 16, 2013 at 01:17 GMT Originally opened as https://github.com/adobe/brackets/pull/5532

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.

njx included the following code: https://github.com/adobe/brackets/pull/5532/commits

core-ai-bot commented 3 years ago

Comment by njx Wednesday Oct 16, 2013 at 19:57 GMT

It looks like a bad CM submodule SHA snuck in again.@larz0, remember to do a git submodule update any time you merge from master or pull an update to the branch before you make new commits.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Oct 16, 2013 at 20:30 GMT

I'm going afk for a few hours, but I'm hoping to wrap up my initial review this evening.

core-ai-bot commented 3 years ago

Comment by njx Wednesday Oct 16, 2013 at 20:50 GMT

@redmunds - I think a couple of the other CSSInlineEditor comments are in your bailiwick.

core-ai-bot commented 3 years ago

Comment by redmunds Wednesday Oct 16, 2013 at 22:55 GMT

I'm done with my changes.@njx Let me know if there's anything I missed.

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Oct 17, 2013 at 02:30 GMT

I'll finish the review in the morning (Eastern time).

core-ai-bot commented 3 years ago

Comment by njx Thursday Oct 17, 2013 at 04:31 GMT

I think I've addressed all the comments so far.

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Oct 17, 2013 at 14:16 GMT

Review complete. Looks good, and I really like the new feature! Only some minor stuff here.

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Oct 17, 2013 at 15:02 GMT

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?

core-ai-bot commented 3 years ago

Comment by redmunds Thursday Oct 17, 2013 at 15:26 GMT

No. Any suggestions on which keyboard shortcut to use?

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Oct 17, 2013 at 15:46 GMT

cmd-shift-N?

We could also override existing shortcuts since this is a contextual shortcut.

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Oct 17, 2013 at 15:46 GMT

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.

core-ai-bot commented 3 years ago

Comment by njx Thursday Oct 17, 2013 at 17:12 GMT

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.

core-ai-bot commented 3 years ago

Comment by njx Thursday Oct 17, 2013 at 17:48 GMT

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.

core-ai-bot commented 3 years ago

Comment by njx Thursday Oct 17, 2013 at 17:49 GMT

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.

core-ai-bot commented 3 years ago

Comment by dangoor Thursday Oct 17, 2013 at 17:54 GMT

Looks good. Merging.