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

Add a header to remove search bar #121

Merged
merged 1 commit into from
Mar 13, 2015
Merged

Conversation

alicebartlett
Copy link

For manuals we need to remove the search box in the page header as we're going to be adding one that provides custom searching within the manual.
This commit adds a new header that you can call from your app that will remove the search box from the top of the page.
You can call it by adding the following code to your applications ApplicationController in a before filter:
set_slimmer_headers(remove_search: true)

Pivotal:https://www.pivotaltracker.com/n/projects/1261204/stories/88435634

This PR defines the header and adds the processor for the header and a test for the processor.

Further work

  • After this header is merged we should start using it in frontend too which currently uses CSS to display:none the top search box on /search and the homepage (I'll open a PR...)

@dsingleton
Copy link
Contributor

It's probably worth adding a test where there are two elements with the id of search. I've seen this happen a few times, so its worth being defensive and defining that behaviour.

(In some places govspeak will create an id on headings, for fragment links, so this isn't always in our control)

Everything else LGTM (and this is really useful 👍

@alicebartlett
Copy link
Author

@dsingleton To clarify, the behaviour you would expect here is that the first search box encountered in the dom is removed but the second one (added by the application) remains?

Should I be using a more tightly coupled selector to the top search box #global-header #search perhaps?

@dsingleton
Copy link
Contributor

Yeah, probably.

How about test mark markup like:

      <html>
        <head>
        </head>
        <body>
          <header id='global-header'>
            <div id='search'></div>
          </header>
          <h1 id='search'>Search</h1>
        </body>
      </html>

With the expected outcome that the search form in the header is removed and the subsequent #search element is untouched?

@alicebartlett
Copy link
Author

Thanks @dsingleton this is really helpful. Going to work in these changes to improve the code and testing.

Just noticed that this PR also doesn't remove quite enough.
It leaves

<a href="#search" class="search-toggle js-header-toggle">Search</a>

Going to make it remove that too.

For manuals we need to remove the search box as we're going to be adding one that provides custom searching within the manual.

This commit adds a new header that you can call from your app that will remove the search box from the top of the page.
You can call it by adding the following code to your application's ApplicationController in a before filter:

`set_slimmer_headers(remove_search: true)`

Pivotal:https://www.pivotaltracker.com/n/projects/1261204/stories/88435634

This commit:

- defines the `header`
- adds the processor
- adds a test for the processor.
@alicebartlett
Copy link
Author

@dsingleton OK, had another punt at this. I've tightened up the selector for the search box.

Chatted with @edds and I'm just gonna scope the

<a href="#search" class="search-toggle js-header-toggle">Search</a>

removal to a[href=#search] rather than add a wrapping div to the header which I can then remove. Both options are a bit icky, but this seems less likely to be removed by accident in the future.

dsingleton added a commit that referenced this pull request Mar 13, 2015
@dsingleton dsingleton merged commit 2e308b9 into master Mar 13, 2015
@dsingleton dsingleton deleted the add-remove-search-header branch March 13, 2015 16:29
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.

2 participants