fix: command palette - #263
Conversation
Sorry, something went wrong.
|
Thanks for fixing this. I completely forgot to follow through with this and get it merged into the browser extension, too. |
Sorry, something went wrong.
|
I would really like to avoid forks as much as possible. Can you open the PR to @shopify/react-shortcuts first? The folks there could review the change better than we can, and it looks very small. Requiring a ShortcutProvider to be used up in the tree without having any type safety that enforces it also doesn't seem that great. @sqs Maybe we should not use @shopify/react-shortcuts after all? |
Sorry, something went wrong.
I didn’t see any other shortcut libraries that fixed your particular concern (typesafety of context), so I don’t think you’d fix that issue by switching. My assessment is that there weren’t any better libraries than this one (see the commit that introduced it for criteria). But if you find a better one or have different requirements, go for it. |
Sorry, something went wrong.
|
Do we need a shortcut library? |
Sorry, something went wrong.
In hindsight, knowing that it needed this change and that in general we have a lot more requirements for shortcuts than the average app, I would actually probably have built our own and not used a library. |
Sorry, something went wrong.
|
Sorry, something went wrong.
ijsnow
left a comment
There was a problem hiding this comment.
I'm fine with merging this temporarily but I'd like to reiterate Felix's question of needing a library for this. I don't think handling keyboard shortcuts is something that should be done by react components as this library is doing. I don't know what all we need out of shortcuts but I think it can easily be handled without a library. For browser extensions, there's even an API built for this https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/commands.
Fix the tslint error before merging though so CI can pass
Sorry, something went wrong.
A regression (#262) which broke the command palette was introduced (but not published). This fixes that regression. - The regression was introduced by @sqs's change to e-c-c here: sourcegraph/extensions-client-common@19df39f - The dependency was updated in this repository by @chrismwendt here: 8616d66 The issue is that we are using a `@shopify/react-shortcuts` package to declare keyboard shortcuts in e-c-c. The package registers keyboard shortcuts via a `<Shortcut>` component which requires that a `<ShortcutProvider>` component be somewhere above it in the React rendering tree, in order to pass down some React context. In the Sourcegraph web application, we provide such an implementation: https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/src/SourcegraphWebApp.tsx#L259:14 Here in the browser extension, however, we do not -- and this causes any `<Shortcut>` component to break due to missing React context data. If we were to publish the browser extension today, the Sourcegraph Extension command palette would be broken outright (clicking on it causes it to disappear). Fixing the issue is tricky: `@shopify/react-shortcuts`'s `ShortcutProvider` component is designed with the assumption that there will only ever be a single React component rendering the entire application. It uses this assumption to only register a single document keyboard event handler, [otherwise it will install multiple global event handlers to the `document` that conflict with one another](https://github.com/Shopify/quilt/blob/master/packages/react-shortcuts/src/ShortcutManager/ShortcutManager.tsx#L21). In most applications, this assumption holds true, but in our situation here with the browser extension this is not true as we inject independent React views into several different locations on the page and multiple expect to be able to add `<Shortcut>`s. Fixing this becomes more tricky because `@shopify/react-shortcuts` does not expose [the `Provider` part of the `React.createContext` result](https://github.com/Shopify/quilt/blob/master/packages/react-shortcuts/src/ShortcutProvider/ShortcutProvider.tsx#L12) -- meaning it is impossible for us to implement our own `ShortcutProvider` that fixes this issue. I've had to temporarily fork react-shortcuts to expose this, and if all looks good I will create a PR upstream for this: https://github.com/slimsag/quilt/tree/publish-provider In the meantime, we will have to use my fork everywhere (if we don't, React context will not grab the right data and it'll e.g. break the command palette on Sourcegraph.com). This does not actually cause us to register shortcuts in the browser extension for the command palette -- but we certainly can now after this change. See also: - sourcegraph/extensions-client-common#60 - https://github.com/sourcegraph/sourcegraph/pull/500 Fixes #262
|
🎉 This PR is included in version 1.18.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sorry, something went wrong.
A regression (#262) which broke the command palette was introduced (but not published). This fixes that regression.
The issue is that we are using a @shopify/react-shortcuts package to declare keyboard shortcuts in e-c-c. The package registers keyboard shortcuts via a <Shortcut> component which requires that a <ShortcutProvider> component be somewhere above it in the React rendering tree, in order to pass down some React context.
In the Sourcegraph web application, we provide such an implementation: https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/src/SourcegraphWebApp.tsx#L259:14 Here in the browser extension, however, we do not -- and this causes any <Shortcut> component to break due to missing React context data. If we were to publish the browser extension today, the Sourcegraph Extension command palette would be broken outright (clicking on it causes it to disappear).
Fixing the issue is tricky: @shopify/react-shortcuts's ShortcutProvider component is designed with the assumption that there will only ever be a single React component rendering the entire application. It uses this assumption to only register a single document keyboard event handler, otherwise it will install multiple global event handlers to the document that conflict with one another. In most applications, this assumption holds true, but in our situation here with the browser extension this is not true as we inject independent React views into several different locations on the page and multiple expect to be able to add <Shortcut>s.
Fixing this becomes more tricky because @shopify/react-shortcuts does not expose the Provider part of the React.createContext result -- meaning it is impossible for us to implement our own ShortcutProvider that fixes this issue. I've had to temporarily fork react-shortcuts to expose this, and if all looks good I will create a PR upstream for this: https://github.com/slimsag/quilt/tree/publish-provider In the meantime, we will have to use my fork everywhere (if we don't, React context will not grab the right data and it'll e.g. break the command palette on Sourcegraph.com).
This does not actually cause us to register shortcuts in the browser extension for the command palette -- but we certainly can now after this change.
See also:
Fixes #262
Testing plan:
Visit any file, click the command palette button, it should actually open and not disappear.
I have tested on: