Global header search #4740
Global header search #4740
Conversation
Thank you Gabriel! So far the testing has been QA testing, primarily in terms of the explore section (where the changes were most significant, because the inline search bar was removed from there). However, if the changes are approved, we may have to update some of the webdriver tests, as this change involves structural changes to the pages themselves that will likely affect some of the tests. There should never be any change in the output of a search itself, just in the search bar interaction (and the now-removed in-page search bar on explore. Of course, please let me know if you can think of any other areas where this might have some side effects! |
nick-next
force-pushed
the
header-search-bar
branch
from
8eea466 to
396f54f
Compare
There was a problem hiding this comment.
Thanks Nick & Pablo- i like this approach.
I'm seeing an error when i run in "custom DC" mode (./run_server.sh -m -e custom):
This might be a little more involved to fix. some potential options:
(1) in custom dc mode, keep the search bar inline on the explore page
(2) update custom dc headers (in server/templates/custom_dc/**/base.html) to include a search bar. Maybe this means attaching the HeaderBarSearch to some new ID in the custom DC navbar, or it could be a pure-flask inlined search bar
What do you think?
| const [queryResult, setQueryResultState] = useState<QueryResult>(null); | ||
| const [debugData, setDebugDataState] = useState<any>({}); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Can you add a comment describing the useEffect?
|
|
||
| const handleStoreUpdate = ( | ||
| store: typeof queryStore, | ||
| changeType: |
There was a problem hiding this comment.
Can you reuse the ChangeType type here?
| setDebugData: (data: any) => void; | ||
| } | ||
|
|
||
| export const useQueryStore = (): QueryStoreData => { |
There was a problem hiding this comment.
Nice job building out this hook!
| @@ -62,7 +61,7 @@ export interface SVScores { | |||
| export interface SentenceScore { | |||
| sentence: string; | |||
| score: number; | |||
| rerankScore: number; | |||
| rerank_score: number; | |||
| @@ -129,6 +131,7 @@ | |||
| {% include 'metadata/labels.html' %} | |||
| {% include 'metadata/routes.html' %} | |||
|
|
|||
| <script src={{url_for('static', filename='queryStore.js', t=config['VERSION'])}} async></script> | |||
There was a problem hiding this comment.
Can you add this to all of the server/templates/custom_dc/**/base.html files?
|
Also i saw a small difference in padding between the search bar in the visualization tools and the knowledge graph (it might be more apparent if you open the pages up locally):
|
Thank you for catching that! The error itself is fixed by including the store in the various base templates, but that leaves the question of how to implement the search bar in explore pages that don't use the standard header. I would go with option 1 for this PR, because having the inline explorer search as an option (tied to the flag) will allow for all cases, including ones where a custom header implementation doesn't have it (and will give us the flexibility to use it if we do want to later). It actually won't be a big task to restore the inline explorer search. It already does work with the header store functionality and so we really would just need to restore it and then tie its display to the flag that determines if the header search displays (if the header search flag is false, we display the inline search). However, it could be nice to also do option 2 as a separate PR, to move towards having the header bar search as the default standard! Let me know if you are happy with option 1 and I will implement that. All the other comments addressed! |
Sorry - I just saw this comment, and will address when we address the explorer page option from the reply in the other comment! |
Option 1 sounds good to me! And agreed that option 2 can come later, but i dont think it's urgent |
nick-next
force-pushed
the
header-search-bar
branch
from
9c32b28 to
321c3f5
Compare
|
"Also i saw a small difference in padding between the search bar in the visualization tools and the knowledge graph (it might be more apparent if you open the pages up locally):" This was because the CSS behind the visualization tools had padding for a different "search-icon" classed entity that happened to have the same class name (and so that was affecting the padding of the search bar icon). Now fixed! The search bar is also back inline (in the epxlore page) in cases where the header search bar is not displayed. Ready for another look! |
There was a problem hiding this comment.
Looks great! Thanks Nick.
One final request: can you remove the navbar search from the /place/* page? The place page has a separate search when you click "Change Place", and we don't want to show users two search bars.
(This will be moot in a few weeks, as we're planning on rolling out a new, re-designed version of the place page that can have the top search bar added back in)
Sorry for being wishy washy on this- after a little more internal discussion, can you leave the navbar search box on the place page in place. We'll separately remove the "change place" search box altogether. |
…h the direction of the flags reversed. The top-level search bar can still be disabled on individual pages with a flag. Implementation into the header version of the search box the functionality previously only found in the body page versions of the search. This commit is a WIP and still needs to be styled.
…h bar is now at the top). Update to styles to reflect layout changes.
…ntent of the store), and functions and states used to subscribe to changes in it are now rolled out into a custom hook.
… store (as well as to provide them to listeners). This means that the globalStore does not need to be accessed directly. An update to the explore application to use this hook.
- the addition of the header store to all custom base templates. - Better reuse of the ChangeType - Description of hook's useEffect functionality.
- the addition of the header store to all custom base templates. - Better reuse of the ChangeType - Description of hook's useEffect functionality.
nick-next
force-pushed
the
header-search-bar
branch
from
321c3f5 to
4fb2029
Compare
## 1) Fix long list of child places on continent place pages On continent place pages, the list of child places will contain places that are more than one level down in containment (e.g, showing municipalities, not just countries). This PR adds a child place type entry for continents to the child place fetch logic. ### Before:  ### After:  ## 2) Add a "show more" toggle when the list of child places is long Sometimes the list of child places is very long, which overwhelms the page. This PR also adds a "show more" toggle when the number of child places is greater than 15. ### Long lists are truncated initially  ### Show full list after clicking "show more"  ### Short lists don't show a toggle  ## 3) Replace place search with link to knowledge graph on current place pages In preparation for the changes made in PR #4740, this PR also removes the place search bar to avoid showing users double search bars, and instead shows the DCID and a link to the knowledge graph entry, fixing a longstanding feature regression. Before:  After: 
Description
The purpose of this PR is to have the search bar in the header on all pages.
Previously, pages by default did not have the search in the header, and had to be flagged in order to provide the search bar. Now, this flag is reversed and pages will contain the header search by default.
The search bar has been removed from the explore page, and the more complicated search functionality provided in that page (such as the ability to programmatically change the current query and provide a debug modal) is now available to the universal search bar in the header.
Related layout updates have been made to ensure that the explore page displays correctly with the internal search now gone, and the debug "bug" button is now aligned nicely (with the global search at the top). The debug modal has also been revamped.
Notes
When the search bar was inline in the explore page, that page could interact and share state with its inline search. This allowed the explore page to provide and programmatically update the query string as well as to provide debugging information, among other things.
We now have to communicate that information from the explore page all the way to the header. Normally, we might consider options like React context, or lifting of state.
However, in this case, the header and the explore page are in totally separate React apps with separate mount points, and that limits our ability to use more traditional methods to communicate this data.
The solution implemented was to create a global "Query Store", that can be updated by a component in one app, and then have those changes provided to a component in another app. The query store provides a hook to facilitate this communication. This allows these otherwise completely independent applications to communicate with each other in a reactive way.