chore(search): Closes #3321 Change contentSearchUIController to better match existing tests #3325
Conversation
|
@k88hudson can i get your eyes on this too to make sure it looks ok |
| @@ -48,13 +54,15 @@ class Search extends React.Component { | |||
| <span className="sr-only"><FormattedMessage id="search_web_placeholder" /></span> | |||
| </label> | |||
| <input | |||
| autoFocus="autofocus" | |||
There was a problem hiding this comment.
we want this only for about:home
There was a problem hiding this comment.
Ah yes you're right, I'll just remove this because it's broken anyways and there's a ticket to do autofocus anyways #2007
| @@ -28,11 +31,14 @@ class Search extends React.Component { | |||
| // this name, and can add the appropriate telemetry probes for search. Without the | |||
| // correct name, certain tests like browser_UsageTelemetry_content.js will fail (See | |||
| // github ticket #2348 for more details) | |||
| this.controller = new ContentSearchUIController(input, input.parentNode, | |||
| "newtab", "newtab"); | |||
| const isDocumentNewtabOrHome = document.documentURI === "about:newtab"; | |||
There was a problem hiding this comment.
from a programmer "or", this should always be true ;) maybe just isNewtab but we'll want it available in render or elsewhere too for autofocus
sarracini
changed the title
| // probes for search. Without the correct name, certain tests like | ||
| // browser_UsageTelemetry_content.js will fail (See github ticket #2348 for more details) | ||
| const healthReportKey = IS_NEWTAB ? "newtab" : "abouthome"; | ||
| const source = IS_NEWTAB ? "newtab" : "homepage"; |
There was a problem hiding this comment.
Let's name this searchSource and also have a comment describing that it's so search engine plugins can correctly attribute referrals especially.
| @@ -50,7 +50,7 @@ describe("<Search>", () => { | |||
|
|
|||
| wrapper.find(".search-button").simulate("click"); | |||
|
|
|||
| const {search} = wrapper.node.controller; | |||
| const {search} = window.gContentSearchController; | |||
There was a problem hiding this comment.
I suppose this effectively tests that we place it on the global. Might make sense to have a test dedicated to just making sure window.gContentSearchController exists after mounting with a description of it being an expected global.
| // browser_UsageTelemetry_content.js will fail (See github ticket #2348 for more details) | ||
| const healthReportKey = IS_NEWTAB ? "newtab" : "abouthome"; | ||
| const source = IS_NEWTAB ? "newtab" : "homepage"; | ||
| window.gContentSearchController = new ContentSearchUIController(input, input.parentNode, |
There was a problem hiding this comment.
Just to be extra safe, an extra comment here saying this gContentSearchController global is to allow existing about:home tests to pass, but potentially could be renamed / refactored when only activity stream is the only about:home.
sarracini
force-pushed
the
gh3320
branch
2 times, most recently
from
ed01241
to
d52c613
Compare
…o better match existing tests
Fix #3321.