Skip to content

UI improvements for the Copilot rename feature #74149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 26 commits into from
Jun 27, 2024
Merged

UI improvements for the Copilot rename feature #74149

merged 26 commits into from
Jun 27, 2024

Conversation

AmadeusW
Copy link
Contributor

@AmadeusW AmadeusW commented Jun 25, 2024 •
edited
Loading

This PR makes UI changes to the Copilot powered inline rename feature

Pictured below: Keyboard shortcut or button mouse click triggers rename and, if applicable (because of Feature Flag), records that from now on suggestions should be retrieved automatically. Next time rename UI opens, suggestions are obtained automatically.

smart rename ui refresh

Specifically, this PR brings the following changes:

  • Use the results panel (instead of the dropdown) in both automatic and explicit modes

    • explicit mode using result panel:
      image

  • Mouse double click to commit rename (from user feedback)

  • Fix issue where selecting rename suggestions may change width of the popup

    • before:
      smart rename ui resize bug
    • after:
      smart rename ui resize fixed

  • Fix strings (hint and tooltip) that were swapped for automatic and explicit modes

    • image
    • image

  • Add delay prior to making the request

  • The button gets purple highlight only to designate its toggled state, i.e. rename is in automatic mode (through feature flag), and user opted into automatic suggestions (through button click or Ctrl+Space)

    • image
    • In light mode, it's a blue border: image

@AmadeusW AmadeusW requested a review from a team as a code owner June 25, 2024 16:55
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 25, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 25, 2024
@AmadeusW AmadeusW force-pushed the dev/amwieczo/renameUiImprovement2 branch from ecda856 to b0456ab Compare June 25, 2024 17:27
CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2024
View reviewed changes
CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2024
View reviewed changes
src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameUserInputComboBox.xaml
<Setter Property="Padding" Value="4,1,24,1" /> <!-- Add 20px to the right over the default template to prevent popup from changing size -->
</Style>
</ComboBox.ItemContainerStyle>
</ComboBox>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skipping all the xaml. maybe @ryzngard can look at this part.

CyrusNajmabadi
CyrusNajmabadi reviewed Jun 25, 2024
View reviewed changes
src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameViewModel.cs

public bool SupportsAutomaticSuggestions { get; }

public bool IsAutomaticSuggestionsEnabled { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc what thread we can call these on.

@ryzngard
Copy link
Contributor

For UI changes please make sure the following happens:

  1. If you added new items make sure to test with accessibility insights
  2. Please include a screenshot/gif of the new UI

@AmadeusW
Copy link
Contributor Author

Please don't force-push reviews. It makes seeing what changed impossible. Note: when you finally submit you can squash if you want.

Sorry, I rebased to integrate with another PR

@AmadeusW
Copy link
Contributor Author

AmadeusW commented Jun 25, 2024 •
edited
Loading

For UI changes please make sure the following happens:

  1. If you added new items make sure to test with accessibility insights
  2. Please include a screenshot/gif of the new UI

@ryzngard ,

  1. No new items added but I'll do a pass with accinsights
  2. Updated PR with gif and screenshots for itemized items

@CyrusNajmabadi
Copy link
Member

Merge commits still work fine for that :-)

Cosifne
Cosifne reviewed Jun 26, 2024
View reviewed changes
src/EditorFeatures/Core.Wpf/InlineRename/UI/SmartRename/SmartRenameViewModel.cs
{
if (this.SupportsAutomaticSuggestions)
{
this.IsAutomaticSuggestionsEnabled = !this.IsAutomaticSuggestionsEnabled;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔Could you explain about this? why it needs to be flipped?

@Cosifne
Copy link
Member

Cosifne commented Jun 26, 2024

Also, target release/dev17.11 if you want to make it into 17.11.
cc @arkalyanms this would be an M2 change

@AmadeusW AmadeusW changed the base branch from main to release/dev17.11 June 26, 2024 23:18
@AmadeusW AmadeusW requested review from a team as code owners June 26, 2024 23:18
@AmadeusW
Copy link
Contributor Author

done, thank you!

@Cosifne
Copy link
Member

Cosifne commented Jun 26, 2024

Ah I retarget the PR. Seems like the base is based on the main branch after the snap. So we need to manually pick change into 17.11

@Cosifne Cosifne changed the base branch from release/dev17.11 to main June 26, 2024 23:28
@Cosifne Cosifne removed request for a team June 26, 2024 23:28
@Cosifne Cosifne merged commit 4ec795f into dotnet:main Jun 27, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jun 27, 2024
Cosifne pushed a commit to Cosifne/roslyn that referenced this pull request Jun 27, 2024
@AmadeusW @Cosifne
UI improvements for the Copilot rename feature (dotnet#74149)
f8eebb9 
* simplify the rename UI

* minor changes to textbox

* restore the ComboBox

* simplify the ComboBox

* alignment, double click

* partially implement the delay

* implement the delay

* fix layout, button highlight

* fix strings

* button highlight now indicates whether suggestions are obtained automatically

* remove unused field

* revert

* add comments

* revert unnecessary change

* remove ICommand and use explicit, documented methods for fetching suggestions and toggling the behavior

* simplify logic

* indent

* PR feedback

* comments

* fix cancellation token, add isDisposed flag

* remove unused property

* revert diagnostic values

* address AccInsights issue

* update accessible name to address a warning

* fix warning

* undo diagnostic change
This was referenced Jul 4, 2024
Closed
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@Cosifne Cosifne Cosifne approved these changes

Assignees
No one assigned
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.12 P1
Development

Successfully merging this pull request may close these issues.
5 participants
@AmadeusW @CyrusNajmabadi @ryzngard @Cosifne @RikkiGibson