This repository was archived by the owner on Nov 6, 2018. It is now read-only.
Merged
Conversation
emidoots
force-pushed
the
use-slimsag-react-shortcuts
branch
from
cdabbc3 to
973f2de
Compare
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
=======================================
Coverage 17.12% 17.12%
=======================================
Files 35 35
Lines 1045 1045
Branches 269 269
=======================================
Hits 179 179
Misses 866 866
Continue to review full report at Codecov.
|
emidoots
changed the title
emidoots
pushed a commit
to sourcegraph/browser-extensions
that referenced
this pull request
Oct 24, 2018
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
4 tasks
emidoots
changed the title
|
🎉 This PR is included in version 10.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
emidoots
pushed a commit
to sourcegraph/browser-extensions
that referenced
this pull request
Oct 24, 2018
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
emidoots
pushed a commit
to sourcegraph/browser-extensions
that referenced
this pull request
Oct 24, 2018
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See sourcegraph/browser-extensions#263 for motivation