-
Notifications
You must be signed in to change notification settings - Fork 445
Expand accordion elements based on URL #552
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| (() => { | ||
| const onChange = (_event) => { | ||
| const urlHash = new URL(document.URL).hash; | ||
| const idFromHash = urlHash.match(/^(#[A-Za-z0-9_-]+)$/); | ||
| if (idFromHash) { | ||
| const accordionHeading = document.querySelector( | ||
| `${idFromHash[1]}.usa-accordion__heading > button`, | ||
| ); | ||
| if (accordionHeading) { | ||
| accordionHeading.click(); | ||
| } | ||
| } | ||
| }; | ||
| document.addEventListener('DOMContentLoaded', onChange); | ||
| window.addEventListener('hashchange', onChange); | ||
| })(); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| require 'spec_helper' | ||
| require 'capybara/rspec' | ||
| require 'rack/jekyll' | ||
| require 'selenium-webdriver' | ||
| require 'yaml' | ||
|
|
||
| Capybara.javascript_driver = :selenium_chrome_headless | ||
| Capybara.app = Rack::Jekyll.new(force_build: true) | ||
| Capybara.server = :webrick |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| require 'capybara_helper' | ||
| require 'pry-byebug' | ||
|
|
||
| RSpec.describe 'accordions on /oidc/authorization/', :js, type: :feature do | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd still prefer actual JS tests instead of or in addition to these Capybara tests. That said, I agree with Jack Cody that this was likely the easiest testing to set up.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure actual JS tests are beneficial when the only output is DOM manipulation. If you had a function like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do agree that the coverage here is good. Compared to some JS testing I've done in the past, doing it this way does feels to me like wearing thick, knitted gloves on my fingers when I'm not used to wearing them. My main concern is that it feels too easy to accidentally break something here in a way that would generate a JS error that this test won't notice. Detecting a JS error like that in a headless browser with Capybara is tricky and fragile. I'm particularly concerned with breaking unrelated behavior on a page where this JS is included but the page doesn't have an accordion (or doesn't have one anymore). |
||
| it 'is not expanded by default' do | ||
| visit '/oidc/authorization' | ||
| expect(page).to have_element('dt') | ||
| expect(page).to have_no_element('dd') | ||
| accordion_button = find('#service_level button') | ||
| expect(accordion_button.native.attribute('aria-expanded')).to eq('false') | ||
| end | ||
|
|
||
| it 'is expanded with an anchor in the URL' do | ||
| visit '/oidc/authorization#service_level' | ||
| expect(page).to have_element('dt') | ||
| expect(page).to have_element('dd') | ||
| definition = find('#service_level ~ dd').text | ||
| expect(definition).to include('acr.login.gov:verified') | ||
| accordion_button = find('#service_level > button') | ||
| expect(accordion_button.native.attribute('aria-expanded')).to eq('true') | ||
|
|
||
| visit '/oidc/authorization#aal_values' | ||
| next_definition = find('#aal_values ~ dd').text | ||
| expect(next_definition).to include('http://idmanagement.gov/ns/assurance/aal/2') | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this came up before I put the code up for review: I deliberately want to keep some input sanitization here. The format I've picked is in line with the general format we use for
classandidattributes, so it feels appropriate to me.