Skip to content
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

add a stackoverflow template #63

Merged
merged 8 commits into from
Oct 9, 2020
Merged

add a stackoverflow template #63

merged 8 commits into from
Oct 9, 2020

Conversation

issamoxix
Copy link
Contributor

I added how many accepted answers are they and it can also give the code of the accepted answer.

Copy link
Owner

@mdolr mdolr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your pull request, I really like the idea of implementing StackOverflow and Stackexchange in general.

I believe they have an API we could use instead of parsing the page like you're doing. Could you use it instead of DOM Parsing please ?

Also your PR has conflicts with master if you could fix those please

@issamoxix
Copy link
Contributor Author

yeah ill try to work with the api

@mdolr
Copy link
Owner

mdolr commented Oct 8, 2020

I just read what you've done with #85, I'm unsure about the fact that the StackExchange API needs a token, I'll look into it more but judging by this thread I don't think a token is needed.

I've looked a bit more into this, I can confirm that not every request needs an API token, as you can see here routes that need an API token are used for posting only, which we don't need. I think you can try to use this routes GET /questions/{ids}/answers and add &filter=withBody

@issamoxix
Copy link
Contributor Author

git_stock
i worked with stackoverflow api

@mdolr
Copy link
Owner

mdolr commented Oct 8, 2020

We shouldn't use insertAdjacentHTML or insert any raw HTML code in the DOM for sanity reasons. Also if we do that the extension won't get accepted on the firefox store. Don't worry I'm currently working on the StackExchange integration using what you've already done and I'm nearly finished

@issamoxix
Copy link
Contributor Author

i can replace it with createTextnode and remove the html from the data

@mdolr
Copy link
Owner

mdolr commented Oct 8, 2020

Well this is what I'm working on right now, I will push it to your PR don't worry

@mdolr
Copy link
Owner

mdolr commented Oct 8, 2020

Could you test this ^ ? What do you think of it ? I've moved everything to files named StackExchanges so we can keep consistency and add more sites like math.stackexchange or apple.stackexchange later. I'm also requesting both the question and answers so we can display some data even if there are no answers.

Do you think we should add something more or are you satisfied with this version ?

@mdolr mdolr added this to the Next release milestone Oct 8, 2020
@issamoxix
Copy link
Contributor Author

I guess that looks good but for the question with answer accepted at least, you should display the most upvoted answer right?
yea that's all
other than that I'm satisfied with this version

@mdolr
Copy link
Owner

mdolr commented Oct 8, 2020

I'm using the answer marked as "accepted answer" on stackoverflow, that could easily be changed by changing the condition here. I'll think I'll merge this tomorrow then as I still want to test some things before merging

@mdolr mdolr merged commit e9e3ac7 into mdolr:master Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants