removing flyout services #5921
removing flyout services #5921
Conversation
github-actions
bot
added
reconciliation
There was a problem hiding this comment.
I haven't tested it yet but there are a couple of places where I suspect you have been a tad zealous ^^ It's just guesswork on my side so I might be wrong though.
In any case it's definitely worth testing the suggest functionality thoroughly after your changes and making sure the visual appearance and functionality is preserved. Did you already do that on your side?
| .fbs-flyout-images, | ||
| #fbs-topic-image { | ||
| background: #fff; | ||
| } |
There was a problem hiding this comment.
Here you are removing not just flyout styling, but also the styling on a lot of other elements: .fbs-pane, for instance, should likely stay. See the syntax for CSS selectors: there is a difference between .fbs-pane .fbs-flyout-pane (which could be removed entirely) and .fbs-pane, .fbs-flyout-pane (which should be simplified to just .fbs-pane).
| .fbs-flyout-subtitle, | ||
| .fbs-topic-properties strong { | ||
| color: #666; | ||
| } |
There was a problem hiding this comment.
Similar problem here (and in the rest of your CSS changes).
| @@ -571,13 +487,13 @@ to inline Internet Explor fixes | |||
| */ | |||
|
|
|||
| * html .fbs-list, | |||
| * html .fbs-list-icons { height: expression( this.scrollHeight > 410 ? "410" : "auto" ); /* sets max-height for IE */ } | |||
| * html .fbs-list-icons { height: expression( this.scrolllHeight > 410 ? "410" : "auto" ); /* sets max-height for IE */ } | |||
There was a problem hiding this comment.
This change is probably a mistake.
|
|
||
| if (!$.suggest.cache) { | ||
| $.suggest.cache = {}; | ||
| } |
There was a problem hiding this comment.
Those three lines above likely need to be kept, I think the cache is still used by the suggest service independently of the flyout.
| }, | ||
|
|
||
| _destroy: function() { | ||
| base._destroy.call(this); |
There was a problem hiding this comment.
I would likely keep this _destroy function since it might be doing things that are unrelated to the flyout functionality, and only remove the flyout-related code in it.
| @@ -1163,15 +1073,6 @@ | |||
| } | |||
| }, | |||
|
|
|||
| hide_all: function(e) { | |||
| this.pane.hide(); | |||
| response_hook: function(response_data, cursor) { | ||
| if (this.flyoutpane) { | ||
| this.flyoutpane.hide(); | ||
| } |
| @@ -1712,15 +1415,7 @@ | |||
| ] | |||
| }, | |||
|
|
|||
| // CSS default class names | |||
| css: { | |||
| item_type: "fbs-item-type", | |||
There was a problem hiding this comment.
Is this item_type flyout-related?
| @@ -466,10 +466,6 @@ img { | |||
| display: none; | |||
| } | |||
|
|
|||
| .fbs-pane, .fbs-flyout-pane { | |||
| z-index: 2000; | |||
There was a problem hiding this comment.
Same comment about CSS rules.
Hey, you are absolutely right! I have now taken care of all your feedback and tested the functionality from my end! |
There was a problem hiding this comment.
Hi @ayushrai206, externals\suggest is a 3rd party library. We should not be altering these files if at all possible:
- externals/suggest/suggest-4.3a.js
- externals/suggest/css/suggest-4_3.css
This is a 3rd party UI library. It is not included in package.json because it was already altered awhile ago with some bug fixes. We want to avoid making any other changes to these files.
Is there some other way to disable the flyout without modifying these files directly?
|
I was also wondering about that… I am not sure to what extent this file is really an external library. I assume it's not maintained by another project (otherwise we would likely be pulling it from npm, no?) and we have already made edits to it, so I wonder if we shouldn't just treat it as an internal file. No strong feelings about this though. In this particular case I think we can avoid making changes to it, by just making sure no flyout settings are passed to it when initializing it on input fields. We'd have to be sanitizing the suggest settings offered in the manifest. |
|
I'd propose to close this PR and do it again with the approach suggested by @elebitzero. The changes should be quite minimal: before initializing the suggest library, we would remove the flyout settings from it. |
|
I agree with @elebitzero 's approach, but just to clarify the provenance of this file, it's a private fork of Freebase Suggest 4.3 which is no longer maintained by Google. While we control the fork, it's best to minimize changes when possible, as it appears to be in this case. |
Fixes #3141
Changes proposed in this pull request: