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

block qtype for a given engine if not implemented #500

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

rupav
Copy link
Member

@rupav rupav commented Feb 10, 2018

Fixes #499

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream master branch.
  • I have added necessary documentation (if appropriate)

Changes proposed in this pull request:

Preview link: https://aqueous-refuge-13068.herokuapp.com/

  • Using jquery to disable qtype for a given engine.
  • Initially (on refresh), its set to as per google search engine, i.e. all except general disabled.

@codecov
Copy link

codecov bot commented Feb 10, 2018

Codecov Report

Merging #500 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   91.84%   91.84%           
=======================================
  Files          30       30           
  Lines         895      895           
=======================================
  Hits          822      822           
  Misses         73       73

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8de3e50...303f980. Read the comment docs.

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

@realslimshanky @umangahuja1 please review.

@umangahuja1
Copy link
Contributor

Looks good to me. Can you share deployed app to test in real time?

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

https://aqueous-refuge-13068.herokuapp.com/ ,Its in the description as well 😃

@umangahuja1
Copy link
Contributor

Oops missed it 😅

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

Please dont search in preview link with parsijoo. It crashes the app.... #498 . Already made a PR to resolve it.

@umangahuja1
Copy link
Contributor

Yeah I found that parsijoo news is not working. So we should disable it.

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

@umangahuja1 no try search iran on news, it will give you results. Its behaviour is dependent on query, its status response is always 200. Thats why #498 handles these cases

@umangahuja1
Copy link
Contributor

umangahuja1 commented Feb 10, 2018

Yeah you are right. So it's good that this issue is already being handled.
As far as this PR is concerned, it looks fine to me.

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

Please approve if it's looks good to you 😀

@umangahuja1
Copy link
Contributor

I don't know how this project takes my approval though. Let's wait for someone more experienced here 😄.
But I feel it disables things nicely and also easy to modify at a later stage when more features will be implemented.

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

@umangahuja1 no problem, you have to go to files changed section, then click on review. From there you can approve or request for changes. Since you have tested my changes and given your time on it, you should do that.

Copy link
Contributor

@umangahuja1 umangahuja1 left a comment

Choose a reason for hiding this comment

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

The issue claimed is resolved in this PR.

Though some news engines like parsijoo and bing need to be addressed separately to resolve crashes.

Copy link
Member

@realslimshanky realslimshanky left a comment

Choose a reason for hiding this comment

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

Although the idea is good, few endpoints have to be considered in order to fix it completely. For example,

  1. When you select an engine which has video enabled say Bing, you click on the video and then you select Google whose video button is disabled, but the previous selection remains intact and doesn't go to default button i.e. general.

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

Nice catch, on it

@rupav
Copy link
Member Author

rupav commented Feb 10, 2018

Updated now, preview link too updated. @realslimshanky

Copy link
Member

@realslimshanky realslimshanky left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM

@vaibhavsingh97 vaibhavsingh97 merged commit 9b00905 into fossasia:master Feb 13, 2018
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.

Block qtypes radio buttons for engines which are yet to be implemented for that engine.
4 participants