-
Notifications
You must be signed in to change notification settings - Fork 75
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
Support for onOpenComponent callback #29
Conversation
🦋 Changeset detectedLatest commit: 44a52e3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -44,12 +44,16 @@ export function ClickToComponent({ editor = 'vscode' }) { | |||
const url = `${editor}://file/${path}` | |||
|
|||
event.preventDefault() | |||
window.open(url) | |||
if (onOpenFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! Thanks for taking the initiative! This definitely seems like a need.
What do you think about moving it up one level before the path
is crafted. Like, something that accepts { editor, source }
?
Such as onOpenComponent({ source })
or similar?
I learned that different editors have different URL schemes, so it'd give the opportunity for people to customize it.
Then, our default implementation would look like:
onOpenComponent = ({ editor, source }) => {
const path = getPathToSource(source)
const url = `${editor}://file/${path}`
window.open(url);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, source
has a file name, line & column. So e.g. for vim
it would be +{line} {file}
. The only drawback here is the user would have to manually import and call getPathToSource
or write their own.
Is there a reason you have editor
in onOpenComponent: ({ editor, source }) => void
? This prop is passed by the same callsite that sets the callback, so they should already know the editor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no real reason. You're right – no sense adding it to an API unless it actually makes sense.
Based off of #26 , I think these are actually two distinct needs. So you can ignore my Qs :)
We'll probably end up with something like:
<ClickToComponent
getURLFromSource={customMapper}
onOpenURL={customOpener}
/>
I've renamed |
@ericclemmons do you think we can move forward with this? Would you rather see a different approach? |
Great, as far as I know this method can support the following four situations:
|
This PR adds support for
onOpenFile
callback. When set, it will be called instead of opening the file with the default editor. This change will enable the support for the long tail of editors and will allow other products to build on top ofclick-to-component
.Usage
Fixes #28