Styling and templates for sitewide search #195

stephen-cox · 2021-06-17T13:12:47Z

…view header

markconroy · 2021-06-17T16:05:47Z

assets/scss/lib/partials/_search.scss

 }
+
+// Search page form.
+#views-exposed-form-localgov-sitewide-search-sitewide-search-page {


Is there a class we can use instead of an id for theming? Following Drupal standards we should not theme by ID.

I have updated this to use classes provided by views instead of the div

markconroy · 2021-06-17T16:07:06Z

assets/scss/lib/partials/_search.scss

+}
+
+// Search results.
+ol.search-results {


We should not need to use the ol element here. Within BEM classes should be styled independent of what element they are placed on.

Adding default styling of an element.class means the base theme that wants to override this will have to use something that is at least as specific as this, if not more specific.

Thanks. This is now nested in the view class and have removed the ol element.

markconroy · 2021-06-17T16:08:00Z

assets/scss/lib/partials/_search.scss

+  padding: 0;
+  list-style-type: none;
+
+  .node--view-mode-search-result {


It would be good if this template had an item in the classes array for this node type called search-result, then we could use that as our Block class in BEM.

Yep - using BEM classes now.

markconroy · 2021-06-17T16:08:50Z

assets/scss/lib/partials/_search.scss

+    margin-top: 0;
+    margin-bottom: 1em;
+
+    h3 {


If we had a .search-result class in the classes array, then we could use .search-result__title here instead of theming a h3 element. Drupal standards say we should not theme by element if at all possible.

Yep - using BEM classes now.

markconroy · 2021-06-17T16:09:59Z

templates/node/node--search-result.html.twig

+* @ingroup themeable
+*/
+#}{%
+  set classes = [


I think we should leave in the default drupal classes that we get with node node--unpublished etc.

Also, we could have a .search-result class here, then the other items can have BEM-based classes such as:

.search-result__title

.search-result__content

etc

Search result template has been updated to user BEM classes and add the default Drupal node classes to the search result wrapper.

markconroy · 2021-06-17T16:11:53Z

templates/node/node--search-result.html.twig

+    <a href="{{ url }}">{{ label }}</a>
+  </h3>
+  {{ title_suffix }}
+  <p>{{ content|render|striptags|raw }}</p>


This should be a div and not a p so we can have more than one field rendered in the search result view mode. For example, an event might want to render the event date as well as a snippet of text.

<div class="search-result__content"> {{ content }} </div>

I think ^ would be better, so we don't need to have specific templates for any other content types, they can all render whatever content the site builders decide they should.

Yep - using a div now rather then a p

markconroy · 2021-06-17T16:12:41Z

templates/views/views-view--localgov-sitewide-search--page.html.twig

@@ -0,0 +1,95 @@
+{#


This looks to me to be identical to a standard views-view.html.twig file. If that's the case, we probably don't need to add it here at all.

The difference between this template and the standard Drupal one is the view filters come before the header. This is so the search block appears at the top and the page and not under the 'Search results' heading.

Doing this in the template seemed the best way to achieve this. Are you aware of any other ways to swap page elements without CSS hacks?

I'm happy with that approach. I have a slightly modified views template in my default setup, where I use a variable to set things like that, then the custom views template just calls the default template and sets the variable to true. You can see a short write-up of it here:
https://mark.ie/blog/printing-regions-in-views-in-different-places-using-the-same-template

stephen-cox · 2021-06-17T16:22:02Z

Thanks for the review @markconroy, I'll go through and address all the issue you found tomorrow.

The theming here is largely just to make things look okay. The actual styling should be reviewed and adjusted by someone who has more expertise than me on this.

stephen-cox · 2021-06-17T16:25:39Z

@cjstevens78 I have a query about the search region in the theme. Do you know why this was hidden in the mobile menu section?

There's a requirement to add a sitewide search form to the header (localgovdrupal/localgov#256) so I moved the header region so it appears after the menus. See:

localgov_theme/templates/system/header.html.twig

Line 48 in 51862af

Is this going to cause problems?

…es (#195)

…ng jobs

stephen-cox · 2021-06-25T11:47:06Z

As the new theme is progressing this is just a stop gap for the demo site until the new theme is ready. Would be good to get this approved with the release of the search.

danchamp

@stephen-cox Happy enough with this for a release given it will be superceded by localgov_base in due course.

cjstevens78

Looks good to me. We dont use the sitewide search on Croydon so doubt it'll conflict.

stephen-cox added 7 commits June 15, 2021 13:53

Added search result template and basic styling (localgovdrupal/localg…

4d7251a

…ov#259)

Compiled CSS for search changes

de9f2bc

Added search page form styling

151078d

Tweaks to sitewide search page

5f5a9d4

Added template for sitewide search view so search form appears above …

da41f97

…view header

Compiled CSS

de9464e

Use article as container for a search result

fff5a04

stephen-cox mentioned this pull request Jun 17, 2021

Site-wide search - default implementation out of the box localgovdrupal/localgov#259

Closed

7 tasks

stephen-cox added 3 commits June 17, 2021 17:08

Further search styling

319aa51

Compiled CSS

de5706c

Moved search region of of mobile header container

51862af

markconroy reviewed Jun 17, 2021

View reviewed changes

stephen-cox requested a review from cjstevens78 June 17, 2021 16:25

stephen-cox added 2 commits June 18, 2021 16:18

Updated sitewide search results styling and template to use BEM class…

a42119c

…es (#195)

Compiled CSS

20268ac

stephen-cox mentioned this pull request Jun 18, 2021

Include localgov_search 1.0.0 localgovdrupal/localgov#273

Merged

Added cache version to Github actions for cache busting when re-runni…

f461b56

…ng jobs

stephen-cox requested a review from danchamp June 25, 2021 11:45

danchamp approved these changes Jun 25, 2021

View reviewed changes

stephen-cox merged commit cbb5fa3 into 1.x Jun 29, 2021

stephen-cox deleted the feature/localgov-259-search-2.x branch June 29, 2021 09:41

stephen-cox mentioned this pull request Jun 29, 2021

1.0.1 Release #196

Merged

cjstevens78 reviewed Jun 30, 2021

View reviewed changes

Styling and templates for sitewide search #195

Styling and templates for sitewide search #195

Uh oh!

Conversation

stephen-cox commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-cox commented Jun 17, 2021

Uh oh!

stephen-cox commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephen-cox commented Jun 25, 2021

Uh oh!

danchamp left a comment

Choose a reason for hiding this comment

Uh oh!

cjstevens78 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stephen-cox commented Jun 17, 2021 •

edited

Loading