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.

Sign up for GitHub

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

Jump to bottom

Fix invalid html generation #302

Merged
merged 2 commits into from
May 2, 2020
Merged

Fix invalid html generation #302

merged 2 commits into from
May 2, 2020

Conversation

dirkjonker
Copy link
Contributor

Debugging some HTML issues with the w3c validator I ran into some errors that were caused by this gem.

  • A second body element is created at the end of the page to add the data-default-select attribute. As the HTML spec only allows for a single body element, this fixes that by adding the attribute to the existing body element.
  • All addon inputs inherit from formtastic's StringInput which adds the attributes maxlength and size to every input. This is also invalid HTML because these attributes are only valid for input with certain types such as text.

@jgmontoya
Copy link
Contributor

Hi, thanks for the PR!
Any chance you could add some tests so we don't run into this in the future?

@jgmontoya jgmontoya linked an issue Apr 25, 2020 that may be closed by this pull request
Two <body> sections in activeadmin views #273
Closed
@dirkjonker
Copy link
Contributor Author

Of course, I added some specs for it, they fail when you check out the current version.

Failures:

  1) Select 2 when building ActiveAdmin html the <body> element is present in the document only once
     Failure/Error: expect(page.all('body').size).to eq 1
     
       expected: 1
            got: 2
     
       (compared using ==)
     # ./spec/features/inputs/select2_spec.rb:85:in `block (4 levels) in <top (required)>'

  2) Select 2 when building ActiveAdmin html the <body> element contains the data-default-select attribute
     Failure/Error: body = find("body")
     
     Capybara::Ambiguous:
       Ambiguous match, found 2 elements matching visible css "body"
     # ./spec/features/inputs/select2_spec.rb:91:in `block (4 levels) in <top (required)>'

Finished in 2.98 seconds (files took 2.19 seconds to load)
7 examples, 2 failures

Failed examples:

rspec ./spec/features/inputs/select2_spec.rb:82 # Select 2 when building ActiveAdmin html the <body> element is present in the document only once
rspec ./spec/features/inputs/select2_spec.rb:88 # Select 2 when building ActiveAdmin html the <body> element contains the data-default-select attribute

@dirkjonker
Copy link
Contributor Author

@jgmontoya are these tests sufficient? Are tests for the input attributes also required?

@jgmontoya
Copy link
Contributor

Looks good, thanks for your contribution!

@jgmontoya jgmontoya merged commit a12af5e into platanus:master May 2, 2020
@dirkjonker dirkjonker deleted the fix-invalid-html branch May 2, 2020 09:00
@dirkjonker dirkjonker mentioned this pull request May 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monkeyci monkeyci monkeyci left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Two <body> sections in activeadmin views
3 participants
@dirkjonker @jgmontoya @monkeyci