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

fix command palette key #4890

Merged
merged 8 commits into from
Aug 20, 2024
Merged

fix command palette key #4890

merged 8 commits into from
Aug 20, 2024

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Aug 17, 2024

Started out as a fix for the command palette key binding. Turned in to a refactor and an alternative centralised way of defining how keys are displayed in footer and key panel.

Fixes #4887
Fixes #4901

Copy link
Contributor

@TomJGooding TomJGooding left a comment

Choose a reason for hiding this comment

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

Thanks Will for taking another look at this!

I don't think this needs a 'fixed' entry in the CHANGELOG, as the command palette binding change wasn't yet released?

However as mentioned in #4857 (comment), there's currently nothing in the CHANGELOG for this binding change. which might be breaking change if apps have their own bindings for Ctrl+p?


class NewPaletteBindingApp(App):
COMMAND_PALETTE_BINDING = "ctrl+backslash"
COMMAND_PALETTE_DISPLAY = "ctrl+\\"
Copy link
Contributor

@TomJGooding TomJGooding Aug 17, 2024

Choose a reason for hiding this comment

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

Perhaps I've misunderstood this new COMMAND_PALETTE_DISPLAY variable , but why does the snapshot show ^\ rather than ctrl+\?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some work in #4876 that will fix that.

Copy link
Contributor

@TomJGooding TomJGooding Aug 17, 2024

Choose a reason for hiding this comment

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

It might be worth adding that work in this PR if possible, as currently the snapshot is not really as expected?

CHANGELOG.md Outdated
@@ -16,17 +16,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/).
- Added "Show keys" option to system commands to show a summary of key bindings. https://github.com/Textualize/textual/pull/4876
- Added "split" CSS style, currently undocumented, and may change. https://github.com/Textualize/textual/pull/4876
- Added `Region.get_spacing_between` https://github.com/Textualize/textual/pull/4876
- Added `App.COMMAND_PALETTE_KEY` to change default command palette key binding
Copy link
Member

Choose a reason for hiding this comment

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

Remember to link this PR

@willmcgugan willmcgugan merged commit 424ef64 into main Aug 20, 2024
18 of 19 checks passed
@willmcgugan willmcgugan deleted the fix-cp-binding branch August 20, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Centralize the key display logic in the App COMMAND_PALETTE_BINDING doesn't change the binding
3 participants