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

USWDS - Search: Use consistent search markup. #3327

Merged
merged 9 commits into from Mar 17, 2020

Conversation

mejiaj
Copy link
Contributor

@mejiaj mejiaj commented Feb 25, 2020

Description

Search styles depend on div[role="search"] being a child of .usa-search. Otherwise it breaks to next line on small screens.

Additional information

image

image

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

@mejiaj
Copy link
Contributor Author

mejiaj commented Feb 25, 2020

@thisisdano, looks like axe is complaining about the search component page where the variations are listed on the same page.

I've tried adding them to their own iframe, but then axe doesn't test them. We could:

  1. Separate into separate components and ignore the combined one.
  2. Ignore the combined one and use the search-header component to test the accessibility of search

Happy to hear any other suggestions though.

@thisisdano
Copy link
Member

I think it is a best practice to put the role=search in the form element, as we changed it in 2.5.0, so I'd prefer to change the stylesheet to style properly whether or not the role is in the < 2.5.0 markup or the 2.5.0+ markup, to support both better markup and backward compatibility.

I'll push this change.

Copy link
Contributor Author

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

LGTM

@thisisdano
Copy link
Member

Went back and reverted all the markup changes so we've focussed only on the styling here. I've added a new issue (#3344) that we can use for normalizing the markup. That work should go in the next minor version and this work could go in a 2.5.2 bugfix release.

<button class="usa-button" type="submit">
<span class="usa-search__submit-text">Search</span>
</button>
<div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this in fractal for me:
image

We need to add the role to the div, remove the div, or update the styling.

Add the search role to the <form> and remove the redundant <div>
I know this is kinda ugly, but we introduced some inconsistent code into 2.5.0 and this ruleset needs to accomodate the pre-2.50, 2.5.0, and 2.6.0 markup
Copy link
Contributor Author

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

LGTM

@thisisdano thisisdano merged commit fa18bdf into develop Mar 17, 2020
@thisisdano thisisdano mentioned this pull request Mar 18, 2020
@mejiaj mejiaj deleted the feature/USWDS-search-fix branch August 18, 2020 20:43
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