-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Feature: implement button for GitHub and Stack Overflow #3
Conversation
0d70884
to
3f660a7
Compare
Great. This extension is really going to help a lot in these website. |
"node": true | ||
}, | ||
"plugins": ["prettier"], | ||
"extends": ["prettier"], | ||
"parserOptions": { | ||
"ecmaVersion": 2020, | ||
"sourceType": "module", | ||
"sourceType": "script", |
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.
We're not currently using modules, so I think this is safer?
{ | ||
"files": ["ext/**/*"], | ||
"env": { | ||
"browser": true |
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.
Scoping the browser env to just the files that are executed by the browser.
"no-else-return": 2, | ||
"no-inner-declarations": 2, | ||
"no-lonely-if": 2, | ||
"curly": "error", |
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.
I'm a maintainer of ESLint, and we generally recommend using "error"|"warn"|"off"
instead of numbers for clarity!
Sorry, diff got a little weird with the renamed files after the rebase. I realized that this doesn't implement the button for the GitHub Issues view, but I think it would be nice to land this and follow up with that if the team is amenable to that. |
So what works now?! (Will be able to test in few) |
Should work on StackOverflow and GitHub Pull Requests (both the conversation and code diff views) and Commit views. |
I'd like to figure out a better way to detect the when to render the buttons for the GitHub view (since editable comments can be created on the fly) so that we can render it immediately (instead of waiting until the user inputs text), but I think this is a good MVP to build on. |
It didn't work for me.. On SO I can see the button, but it doesn't seem to do anything, and can't see it on GitHub.. |
are you sure you committed all the files? |
Ah, I'm seeing it go in and out on GitHub - sometimes it works and sometimes it doesn't. I assumed it was all prerendered because it feels blazing fast to me and I was seeing it work locally all the time, but it's most likely that sometimes the element it's binding to isn't on the page when it runs. Will probably have to do something similar to the SO one and poll for the element to be in the document. |
It's a great start for sure 😊 |
Can you see if that fixes it for you? The Prettier button should appear when you start editing a comment. I think we can definitely come up with a better check that makes the button appear when the comment appears (would be a nicer UX), but I thought this was a good first pass to get the basic functionality :) |
The button disappears when answering anonymously on SO |
So the button works (on SO) when there is code in the answer.. otherwise it doesn't formats the markdown (for example fixing the links and lists, spaces). |
This is fixed! I didn't realize you could post anonymously on SO 😅 |
I think it's a pretty solid start.. Merging it if you don't have anything to add at this point. |
Sounds good! Definitely lots of room for improvement :) |
Do you mind adding issues with things needs to be done or improved, optimized whatever you think of so we won't forget anything. |
Already on it 😄 |
Also, thanks for the quick reviews! ❤️ |
I took a crack at implementing this! To get this working, I downloaded and added the standalone build and the parsers from
https://prettier.io/lib/*.js
. This obviously isn't ideal, but I figured the team would know best how to go about getting the standalone build in the extension!Some other notes:
ext/_locales/en/messages.json
is necessary because adefault_locale
is listed inmanifest.json
. So far, this is a very easy thing to translate (there's only one string to display!), and it seems like it would be worthwhile.For reference, here's what it currently looks like:
Stack Overflow:
GitHub: