GitHub/Gist code fetching & automatic highlighting #252
Conversation
|
Nice! I'll take a look and try it out this week. I'll comment a few things that stand out now though in a quick review |
src/editor/controls/Sidebar.tsx
Outdated
| setAttributes({ | ||
| code, | ||
| enableHighlighting: Boolean(lineNumbers), | ||
| lineHighlights: Boolean(lineNumbers) ? `[${lineNumbers.startLine},${lineNumbers.endLine}]` : '', | ||
| }); |
There was a problem hiding this comment.
I believe this should only be updating the code right?
There was a problem hiding this comment.
So those other lines are there to allow users to be able to preserve highlighted lines from GitHub links. Would you like me to remove that capability from this PR?
There was a problem hiding this comment.
Oh I didn't realize that. That's interesting. I like the idea actually.
I think that makes sense for the first time it fetches, but not for syncing long term though
Could it work like this?
- User pastes the link
- We fetch the code
- Parse it to get line numbers
- Remove the line numbers from the URL
- Add line number highlights and URL (which no longer includes line numbers reference)
- Then when syncing next time, next week, etc we just sync the code with no line numbers
So we just have it as is, but we dont persist the url with the line numbers. that way it only ever adds the highlights once. I'm just thinking about users that want to sync with a gist they edit, and the line numbers might now always be the same. i'd hate for them to need to add the URL.
Alternatively we could add a checkbox like "preserve line highlights" or something.
What do you think?
| if (isValidUrl(value)) { | ||
| inputRef.current?.classList.remove('cbp-input-error'); | ||
| } else { | ||
| inputRef.current?.classList.add('cbp-input-error'); | ||
| } |
There was a problem hiding this comment.
Should use a react approach here and either just add a ternary on the component, or use the classnames package.
There was a problem hiding this comment.
So unfortunately you can't really set the className on the TextComponent and have it apply the styles to the input by itself (maybe I need to submit a PR for that too lol)
I went ahead and changed it to a ternary within a useEffect. Would you like me to still use the classnames package?
There was a problem hiding this comment.
Maybe just make the text red? You could use a
tag directly instead of the help prop. Might need to add something like m-0 p-0 -mt-1 also if it's too low.
The issue with directly manipulating the dom is that react doesn't track those changes so it could bug out when doing a diff (if not today because conditions are right, maybe in 2 years something changes).
Also, for the text, I was thinking about this a bit more. What do you think about:
- The main panel is labelled "Sync" instead of "GitHub"
- The inner control title is "GitHub" instead of "Github Repo Link"
- The help text is "Supports GitHub file links and Gists"
- The button says "Save" instead of "Fetch Code" and I guess behaves the same
I also think maybe adding the highlights makes it a bit confusing. If the goal is to keep a file in sync (in case they change the Gist, for example) it would update and put any highlights out of place. It might even be worth disabling those features entirely (even disabling editing too actually). What do you think?
- ignore any line highlisghts entirely (so no need to parse the url at all)
- At the top of the panel add a short note on the expectations, that this will override any code edits and forcefully sync the file each page load.
Thanks again, and hope this isn't asking too much. It's great so far :P I'm happy to work on it too though.
There was a problem hiding this comment.
and hope this isn't asking too much
Not at all! In fact I really appreciate all the time you're taking to instruct me about your preferences! I'm learning quite a bit, of course I want to be respectful of your time too so if you want to take ownership of something to push things along feel free! This is your repository after all 👍
I also think maybe adding the highlights makes it a bit confusing. If the goal is to keep a file in sync (in case they change the Gist, for example) it would update and put any highlights out of place.
Yknow, while working on the other code change around highlights, I'm realizing now that this is really prone to accidental overwrites. If I put myself in the place of the writer/user, I really want things to stay "deterministic". I don't want simply typing into the GitHub textbox to overwrite my changes, I would probably much prefer to only overwrite my code when I click the "Save" button.
Same thing with code highlighting, because now there would be 2 places where highlighting could change.
I would have to agree with the option with removing the features! Apologies for the complications 😅
There was a problem hiding this comment.
Hmm, I am having trouble trying to only set the code when the user clicks the "Save" button though. Since I am using the hook at the top of my component it keeps fetching/setting the remote code.
There was a problem hiding this comment.
I would probably much prefer to only overwrite my code when I click the "Save" button.
So the question is actually what is the feature, or what problem is this feature solving? I believe we're seeing it a bit differently so let me write out summaries of both and get your opinoin.
- It's solving the problem of importing code into the page. So instead of copy/pasting the code into the editor, we are giving them a text input and a button to copy/paste the url and fetch it for them. Additionally, they can go and refetch if they press the button again later (e.g. in 6 months). To me this implies the source file will not be changing that often, so the feature would be "to help users import the code"
- It's solving a problem for users that manage an actively changing gist or file in a repo that they want to keep in sync without much effort. In my head I pictured having 5-6 on a page and they all just sync when the user goes to edit the page (caveat being it requires they edit the page for it to re-sync). So the feature in this case is "to help auto sync with code that is managed elsewhere"
My descriptions may be biased too, but even writing them out it's not super clear that this will be that useful of a feature if it can't auto-sync behind the scenes (like once an hour or something) even when the user is not editing the page. I also hesitate to give a feature that implies it does something it doesn't, if you know what I mean.
If the idea is to provide easy imports (which I'm not against either), I'd almost rather it incorporate the github oath flow and use the GH api to load in recent gists/repos in the sidebar similar to how selecting a theme works (code previews load in and they can press to import).
Since I am using the hook at the top of my component it keeps fetching/setting the remote code.
I can check the code but typically you would use a useEffect and then update the code when the data changes, and you wouldn't fetch the data until a URL existed. So you'd need an intermediary state variable to handle the form input that didnt't feed into swr
|
I made it so that the line highlighting is only set if it was not set previously. Lemme know what you think 👍 |
|
That sounds right. I'll probably have some time tonight or tomorrow to look at the code again. Been a busy week |
|
I'll write up a response to your previous comment tonight, but I did want to put out there that I would be very open to pairing on this together. That way we could get in sync pretty quickly in the event my response is confusing or if clarification is needed 👍 If you don't think it's necessary that's fine as well |
|
Another idea I was thinking is that we could make the plugin "pluggable" and this sort of feature could be another plugin add-on, like "Code Block Pro - Import From GitHub" and if you want I could help you get it published to w.org as a plugin you manage and of course have creative control over. Edit: If we land on this idea i'd happy link to it in the readme as well |
|
Ohhhhh I think that idea would work quite well actually! After some cursory searching, it seems as though Gutenberg is set up quite well for this kind of thing via their |
|
Well actually I think it would involve just adding a Then you'd use |
|
I have a PR with the slotfills and wrote up some instructions on adding a plugin if you're interested in this route. https://gist.github.com/KevinBatdorf/de687d5e620dfe2af7645b35b917cd87 You can actually filter attributes too (to add a |
|
Any thoughts on this? I understand if you don't want to take that approach. Should I close this for now? Can revisit it another time if you'd like. |
That approach is perfect actually! Allows us to develop our respective changes independently! Now, the commit history on this thing is probably killer hahaha. I propose we close this PR and I'll ping you once I develop the GitHub plugin 👍 Apologies for the long break, I took a vacation so I wasn't paying attention to my emails or anything 😅 Then I caught COVID. Feeling a lot better now |
|
Ahh that's no good. Worst way to end a holiday 😩 I'm traveling next week actually and worried about that too. Probably I need to be extra careful. Glad you're doing better now though. Ok I'll close this and yeah just keep in touch about any updates. |
This PR closes #246
Notes:
GitHubRepositoryControlraw.githubusercontent.com,gist.githubusercontent.com, andgithub.comThis might be overkill, but one immediate extension I could see being made to this is allowing code from sources like GitLab or something, but that's a problem for the future.
I did intend to implement some cypress tests, but I for the life of me could not figure out how to get an environment set up. I can figure it out, but I thought it would be nice to get some feedback while I figure it out.