Skip to content
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.

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

Improves Main Interpreter Picker #6363

Merged
merged 15 commits into from
Feb 21, 2025
Merged

Conversation

samclark2015
Copy link
Contributor

@samclark2015 samclark2015 commented Feb 14, 2025

Adds more useable interface for primary interpreter picker.

Addresses #6150, #3596, #2143

Screen.Recording.2025-02-14.at.10.13.45.AM.mov

Release Notes

New Features

  • Adds action to open Quick Pick to allow user to choose active runtime session, or to create a new one
  • Replaces main interpreter picker dropdown with button to trigger Quick Pick above
  • Modifies Positron action bar button to allow display of custom icon (vs. only Codicons as was previously) to support language icons in new interpreter button

Bug Fixes

  • N/A

QA Notes

e2e: @:interpreter

@samclark2015 samclark2015 self-assigned this Feb 14, 2025
Copy link

github-actions bot commented Feb 14, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:interpreter

readme  valid tags

Copy link
Contributor

@dhruvisompura dhruvisompura left a comment

Choose a reason for hiding this comment

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

Had a couple comments/questions! The overhaul is such an improvement over the old dropdown we had!

Comment on lines 105 to 108
className={positronClassNames(
'action-bar-button-icon',
props.dropdownIndicator,
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the props.dropdownIndicator is being evaluated to its string value but I only see .action-bar-button-icon.enabled-split defined in the css file. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure - I stole this from the original Codicon implementation from @softwarenerd (line 96 sbove)

@samclark2015 samclark2015 marked this pull request as ready for review February 19, 2025 22:00
dhruvisompura
dhruvisompura previously approved these changes Feb 19, 2025
Copy link
Contributor

@dhruvisompura dhruvisompura left a comment

Choose a reason for hiding this comment

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

Looks great! I had one small suggestion

softwarenerd
softwarenerd previously approved these changes Feb 19, 2025
Copy link
Contributor

@softwarenerd softwarenerd left a comment

Choose a reason for hiding this comment

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

I left a couple of ideas.

@samclark2015
Copy link
Contributor Author

@juliasilge Hoping to get your feedback re: verbiage here. We are using "Runtime" in this new quickpick, but are curious if "Session" is better fit in advance of the multisession support? Fwiw, this is behind the multisession feature flag.

If yes, I'd propose these changes:

  • "Select an Active Runtime" -> "Select a Session"
  • "All Runtimes..." -> "New Session..."
  • "Start Another Runtime" -> "Start a New Session"
Screenshot 2025-02-20 at 9 06 48 AM Screenshot 2025-02-20 at 9 06 54 AM

dhruvisompura
dhruvisompura previously approved these changes Feb 20, 2025
Copy link
Contributor

@dhruvisompura dhruvisompura left a comment

Choose a reason for hiding this comment

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

LGTM - awaiting any naming changes! 🚀

juliasilge
juliasilge previously approved these changes Feb 20, 2025
Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

This is the first time I've played with some of this new UI coming together and it is really nice. 🤩

I agree that we should change some of this wording to use "session"; a benefit of this is that folks have found runtime vs. interpreter confusing and this reduces our use of that in favor of the more straightforward "session".

@samclark2015
Copy link
Contributor Author

Updated wording using "Session" vs "Runtime":

Screenshot 2025-02-20 at 5 18 54 PM Screenshot 2025-02-20 at 5 18 58 PM

…/topActionBarInterpretersManager.tsx

Co-authored-by: Dhruvi Sompura <[email protected]>
Signed-off-by: Sam Clark <[email protected]>
Copy link
Contributor

@dhruvisompura dhruvisompura left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@dhruvisompura dhruvisompura merged commit c4908fd into main Feb 21, 2025
8 checks passed
@dhruvisompura dhruvisompura deleted the feature/runtime-picker-rework branch February 21, 2025 01:16
@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants