[MRG+1] Added v2 Custom Search API and fixed css placement for search box #8542
Conversation
|
This looks much better than what is on master. +1 for merge! To help another reviewer, here is the link to the website generated by circleCI: |
|
I'm getting a very buggy rendering in Chrome. The search box is half off the screen (vertically) and "powered by Google" floats over the navbar ("Home" et al) the whole time. |
|
I am seeing the same now. Strangely, I am pretty that I did not see this when I did the initial review. I am removing my +1 until we figure out what is happening. |
😕 I am still seeing the original problem, what I am missing ? Here is a snapshot: I tried to generate the doc locally to make sure this was not a CircleCI quirk and I still see the same issue as well ... |
|
This is really weird 😅 It is as if Google changed the api's working within a day. |
|
Sigh. Which points to the underlying problem that we're having designs
around whatever Google gives, but has not promised, us
…On 7 Mar 2017 5:26 pm, "Parminder Singh" ***@***.***> wrote:
This is really weird 😅 It is as if Google changed the api's working
within a day.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8542 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz6wGflVoXEw5dEz9FrtqpGFRX0UPYks5rjPihgaJpZM4MUOUg>
.
|
|
I can get a somewhat decent formating with the following CSS: .search_form {
margin-top: -35px;
min-height: 42px;
max-width: 19em;
margin-left: auto;
margin-bottom: -5px;
}
It should be more robust than what we had before. |
| @@ -219,10 +219,6 @@ div.navbar div.nav-icon { | |||
| width: 50px; | |||
| } | |||
|
|
|||
| .gsc-branding { | |||
| display: none; | |||
There was a problem hiding this comment.
In order to remove "Powered by Google", keep this section and add !important at the end of this line. From what I can gather Google his adding its own CSS dynamically and we want our own CSS to prevail. Adding !important is one way to ensure that.
|
Added the !important and changed values for css for a bit prettier solution. |
|
Link to the generated doc: http://scikit-learn.org/circle?9369 |
|
LGTM. It would be great to fix the vertical misalignment of the input box and the search button in a separate PR ! |
|
😕 it looks like something has changed now and the text color in the "Search" button is white. There seems to be a way to disable additional css, e.g. from: I think this is worth investigating. |
|
😅 Google reverted the changes that happened last time.... 😓 The disabling of css won't work as google loader is not used for the cse v2. There is a way to load results from a custom form |
In any case we need to find a way to be robust against this kind of changes ... |
|
In any case we need to find a way to be robust against this kind of changes ...
My suggestion is more robust (it cannot be completely robust) as it
encloses the search in a rather tight box.
|
|
I am thinking of making a custom implementation for as much css as possible and override them all. That would make the search more reliable on styling even if google changes it |
…pi and new api alike
Codecov Report
@@ Coverage Diff @@
## master #8542 +/- ##
=========================================
+ Coverage 95.48% 95.5% +0.01%
=========================================
Files 342 342
Lines 61000 61207 +207
=========================================
+ Hits 58246 58454 +208
+ Misses 2754 2753 -1
Continue to review full report at Codecov.
|
|
I settled values in middle of old and new so it still looks ok if google randomly changed response to old api. 😛 Added Sklearn color to the search button and it looks very cool! @GaelVaroquaux I didn't use max-width as results and search box are in same div so that made the result enclosed in a small box of 19em width. |
|
Issues:
|
|
I am not a CSS expert but the result LGTM Link: snapshot (on Firefox): |
|
LGTM |
|
Thanks @Trion129 :) |
|
This is a cool way to reference the result... |
|
(@jnothman I merged it as all the mentioned issues have been fixed in the latest commit) |
|
thanks. I've not had time to look
…On 13 Mar 2017 7:13 am, "(Venkat) Raghav (Rajagopalan)" < ***@***.***> wrote:
***@***.*** <https://github.com/jnothman> I merged it as all the mentioned
issues have been fixed in the latest commit)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8542 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEz63sqSzil27WzpqPlRrMYORScf4whks5rlFHcgaJpZM4MUOUg>
.
|
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
|
Still seeing this issue, and agree that it is confusing. |
|
https://scikit-learn.org/circle?9413 is looking good though 😅 |
@thundergolfer the issue should be fixed in the dev doc: http://scikit-learn.org/dev. |
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
… box (scikit-learn#8542) * Added v2 Custom Search API and fixed css placement for search box * Tweaked CSS and hid the branding section * Button made cute and override some css to make it look good for old api and new api alike * Better button placement * Colored the search results and pagination section
Reference Issue
Fixes #8388
What does this implement/fix? Explain your changes.
I changed the old Custom Search API with v2 as the one used currently was deprecated. That fixed the Powered by Google. This caused wrong placement of search box though, so I changed the earlier value in css to fix the positioning.