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

feat: nav bar with search bar #4

Merged
merged 6 commits into from
Oct 31, 2024
Merged

feat: nav bar with search bar #4

merged 6 commits into from
Oct 31, 2024

Conversation

hroth1994
Copy link

@hroth1994 hroth1994 commented Oct 31, 2024

πŸ”— Linked issue

https://energysage.atlassian.net/browse/CED-1940

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Add a search bar and documentation that uses the design of the MVP and fulfills these requirements:

  • Search button should be display: none by default, and have a specific id on it, so that VWO can target it for showing
  • Search button should be fill: blue-600 while the dropdown is open
  • When dropdown is opened, should automatically put the focus on the textbox so the user can start typing right away for accessibility
  • Put an aria-label on the search input element itself
  • The overlay should not appear on search button hover, should appear while the search bar is open, and clicking outside the search bar should hide the overlay and close the search bar
  • Submission of the search form should result in a GET form submission to /search/, meaning that the search query is encoded as a query parameter.

πŸ₯Ό Testing

🧐 Feedback Requested / Focus Areas

  • General
  • VoiceOver is currently not working on the search icon button, but the nav bar in general is not accessible and needs to be rewritten

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.
  • I have documented testing approach

@mpleroux
Copy link
Member

πŸ‘€

Copy link
Member

@mpleroux mpleroux left a comment

Choose a reason for hiding this comment

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

This all looks great.

I checked the ticket requirements and reviewed the new layout and SCSS changes.

The navbar with search button works well in every responsive breakpoint I tried, and tabbing between elements did what I expected when the search form was open or closed.

Copy link
Collaborator

@nathanielwarner nathanielwarner left a comment

Choose a reason for hiding this comment

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

Looks good! The one thing that I'm noticing is that it's not working properly with VoiceOver. It'd be nice to fix that if it's easy, but the nav as a whole is not accessible (and will hopefully be rewritten fully soon), so I'm not too worried about getting this perfect.

@hroth1994 hroth1994 merged commit 18c9ef8 into main Oct 31, 2024
1 check passed
@hroth1994 hroth1994 deleted the ced-1940-nav-search-bar branch October 31, 2024 19:52
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.

3 participants