Some code actions ordering and cleanup #11659
Conversation
Currently its "Extract to New Component" which is annoying, and unlikely to be correct IMO
Without this fix, depending on where in the line the cursor is, sometimes the first code action is "Extract to Code Behind", and sometimes its "Extract to New Component". The difference is pretty jarring. Before we had "Extract to New Component", there would simply be nothing offered sometimes, but now people see the lightbulb and muscle-memory their way to the wrong action.
Missed these on the first pass because they didn't fail. Hopefully I got the fixes right :D
| Priority = VSInternalPriorityLevel.High, | ||
| Name = LanguageServerConstants.CodeActions.AddUsing, | ||
| // Adding a using for an existing component should be first | ||
| Order = -1000, |
There was a problem hiding this comment.
💡 Should these be in some kind of constants file? That way it's easier to compare across actions which comes before another and what space is available if needed
There was a problem hiding this comment.
IMO no, but I'm happy to go with majority rules if someone wants to move them. My arguments are a) They're only specified in this file anyway and b) I expect setting Order to be the exception not the rule.
I thought about porting over Roslyns [Extension(After = ...)] but I thought about it for a bit and decided it would make things hard to navigate and reason about. IMO having the 3 things that set an order all be in this one file makes it easy to see.
There was a problem hiding this comment.
That's fine with me. It's just something that will wait until someone pushes it into being needed (and outside this file)
| Data = data, | ||
| TelemetryId = s_createExtractToComponentTelemetryId, | ||
| Name = LanguageServerConstants.CodeActions.ExtractToNewComponentAction, | ||
| // Since Extract to Component is offered basically everywhere, always offer it last |
There was a problem hiding this comment.
I wonder if we should reduce where it's offered without a selection...
There was a problem hiding this comment.
I could see that argument, but I do also wonder if this is a transitional point in time. If you haven't seen it, the reddit poster mentions specifically that they see the light bulb in their peripheral vision, and assume it must be the code action they want. This would have been reliable in the past in Razor, but I don't think its something we should try to get back to, or rely on for usability. I would assume that people don't have that same behaviour in C# files, because there is usually something that is offered, and I'd love to think Razor will eventually get to that too.
| [InlineData("@cod[||]e {")] | ||
| [InlineData("@code[||] {")] | ||
| [InlineData("@code[||]{")] | ||
| public async Task WorkAtAnyCursorPosition(string codeBlockStart) |
There was a problem hiding this comment.
I've been thinking about this for other tests.... I wonder if we should have a way to say Anywhere in this span should work with any selection/single cursor position. Not worth doing in this PR but food for thought
There was some feedback on Reddit this morning about code action ordering and Extract to Component being a little "in your face". See https://www.reddit.com/r/Blazor/comments/1jjg3bh/am_i_doing_something_wrong_or_is_intellisense_and/mjq7jp3/?context=3 and the rest of the thread in general.
This PR:
$$@codesince previously there would be no light bulb in that situation, but now there is and muscle-memory makes people hit it, which extract the code block to a new component, which is not what they want.Note that the ordering here is only among the Razor code actions. All C# and Html code actions are always still after the Razor code actions, even Extract to Component.