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 in-page filtering to implementation record #4532

Merged
merged 2 commits into from Mar 5, 2024

Conversation

andysellick
Copy link
Contributor

What

Adds filters to pages on the GA4 implementation record to allow users to 'search' through lists of events and attributes.

I've tried to write the script as generically as possible so it can be applied to other things if needed. It's written as a GOVUK module, so is initialised by the modules script in the gem. It uses data attributes on the list of things to filter. For the list of events by type I've extended the script to check for titles and items, and hide the titles if all of the items in that section are hidden. I've also included the name of the event in a hidden span to improve the filtering (if you're searching for the title, it matches the children, e.g. the select_content events aren't actually called that, but if you search for that you'll find them).

I'm not entirely happy with this code for a few reasons, but it might be the best we can manage for now. Specifically:

  • there's no test for the JavaScript, because there are no JavaScript tests for the dev docs. Setting them up would involve installing and configuring Jasmine and including it in the build pipeline, which seems like a lot of effort to go to for one script. One alternative was to put the script in the components gem (where writing a test would be easy) and there's already a similar filtering script that this could replace (for filtering the list of components in the component guide) but I wanted to put it in /component_guide because it relates to that, but scripts in there aren't available to import from the gem currently.
  • the nice icon for the filter form (input component from the gem) wouldn't display, I think because of something unusual about how components are rendered in the dev docs compared to our usual Rails applications. The workaround I came up with was to put a copy of the search icon in the stylesheets directory, where the CSS is looking for it. It's not ideal, but I didn't want to write a style override to point to a copy of the image somewhere more sensible. An alternative could be just to do without it.

Why

Part of a number of changes to improve the usability of the implementation record.

Visual changes

Screenshot 2024-03-05 at 09 22 44

Trello card: https://trello.com/c/m6vL7lJy/186-add-filters-to-events-and-attributes-pages-on-implementation-record

@andysellick andysellick requested a review from AshGDS March 5, 2024 09:34
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, LGTM 👍

This is irrelevant to this PR, but the Cookies link in the footer 404's on the analytics page, as it's trying to go to /analytics/cookies instead of /cookies. Thought it might be worth fixing in this PR but I'm happy for it to be a separate PR that one of us does.

return
}

this.$module.input.addEventListener('input', function (e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: since e is never used, does it need to be defined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, have removed.

FilterList.prototype.init = function () {
// find the input element
this.$module.input = this.$module.nodeName === 'INPUT' ? this.$module : this.$module.querySelector('input')
if(!this.$module.input) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: standardx would add a space between if( here. E.g. if (!this.$module.input) {

Slightly off topic, but we could add standardx to this repo by running yarn add standardx and then adding this to the package.json - but I guess as it's not a part of the CI it would only be useful for devs.

  "standardx": {
    "ignore": []
  },
  "eslintConfig": {
    "rules": {
      "no-var": 0
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have changed. Yeah, standardx could be useful, let's think about it for a separate PR.

- forgot to update this as part of the move to put analytics on the whole of the dev docs, not just the implementation record
@andysellick
Copy link
Contributor Author

@AshGDS have fixed following your comments and fixed the cookies link in the footer - good spot, thanks.

@andysellick andysellick merged commit 195e422 into main Mar 5, 2024
7 checks passed
@andysellick andysellick deleted the analytics-add-filter branch March 5, 2024 10:47
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.

None yet

2 participants