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

align command palette prefix > to left when visible #8279

Conversation

bhaskarshankarling
Copy link
Contributor

@bhaskarshankarling bhaskarshankarling commented Nov 15, 2020

Fixes the clear button to clear the typed command not clickable in the
command palette.

  • From the primary investigation it looked like the TextBlock element
    introduced in Swap the command palette modes for the prefix > #7935 was somehow blocking (appearing on top of) the
    clear button.
  • It was also blocking the command palette input field from being able
    to access which was preventing the text in the input field from being
    selected and the cursor would still show as pointer cursor instead
    of a text selection cursor
  • Adding HorizontalAlignment="Left" property to the above-mentioned
    TextBlock element fixed the issue.

Validation Steps Performed

  • Created the Dev build for x64 in Visual Studio and verified the
    functionality manually.

Closes #8220

@ghost
Copy link

ghost commented Nov 15, 2020

CLA assistant check
All CLA requirements met.

@bhaskarshankarling bhaskarshankarling changed the title fix(command palette): align command palette prefix > to left when vis… fix(command palette): align command palette prefix > to left when visible Nov 15, 2020
@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Nov 15, 2020
@bhaskarshankarling bhaskarshankarling marked this pull request as ready for review November 15, 2020 12:42
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

what the heck

Testing this out, it definitely works, but like, why?

(That's a rhetorical "why" - I don't really need an explanation)

@bhaskarshankarling
Copy link
Contributor Author

@zadjii-msft It may be possible that the solution could be bit out of normal as I have zero working experience in building Windows applications, nuget, XAML, C++ codebase, PowerShell or Visual Studio project but since I rely heavily on Windows Terminal (and WSL2) for Ruby on Rails and JavaScript projects I really appreciate you folks' effort on making Windows Terminal better everyday. So, I thought of contributing as much as I can even though I don't have previous working knowledge in these tech stack.

So coming back to the initial point, I arrived at the solution mostly by git bisect as the solution was working in the previous version and skimming through some docs to understand XAML and Visual Studio projects just enough to understand the repo. So, please suggest if the fix needs to be implemented in a certain way.

On top of all of this, it would be my first ever open source contribution if it makes to main branch. Thank you very much for approving. If possible please share the links to any resource which you consider would be a good place to start with to understand Windows application development ecosystem.

@zadjii-msft
Copy link
Member

You did a great job with this, I'll be honest. I don't think I would have intuitively tried the HorizontalAlignment=Left thing, which definitely seems to work here. I don't have all that much XAML experience though, so I guess I don't know how that interacts with the width of the control. So props to you for figuring that out 🎉

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I'm surprised that this was the problem. Good catch!

@DHowett DHowett changed the title fix(command palette): align command palette prefix > to left when visible align command palette prefix > to left when visible Nov 16, 2020
@DHowett DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. AutoMerge Marked for automatic merge by the bot when requirements are met labels Nov 16, 2020
@ghost
Copy link

ghost commented Nov 16, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit a8f3f58 into microsoft:main Nov 16, 2020
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Nov 20, 2020
DHowett pushed a commit that referenced this pull request Nov 20, 2020
Fixes the clear button to clear the typed command not clickable in the
command palette.

- From the primary investigation it looked like the `TextBlock` element
  introduced in #7935 was somehow blocking (appearing on top of) the
  clear button.
- It was also blocking the command palette input field from being able
  to access which was preventing the text in the input field from being
  selected and the cursor would still show as `pointer` cursor instead
  of a `text selection` cursor
- Adding `HorizontalAlignment="Left"` property to the above-mentioned
  `TextBlock` element fixed the issue.

## Validation Steps Performed
- Created the Dev build for `x64` in Visual Studio and verified the
  functionality manually.

Closes #8220

(cherry picked from commit a8f3f58)
@ghost
Copy link

ghost commented Nov 20, 2020

🎉Windows Terminal Preview v1.5.3242.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command palette clear text button doesn't work
3 participants