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 the role=button to search #7594

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DivyanshuLohani
Copy link

Add role=button to search on the home page #7575

What does this PR do?

This PR adds role=button to search on the main navigation bar on the homepage
Fixes #7575

image

How should this be tested?

  • Login and go the dashboard
  • Inspect the search button

Required

  • Filled out the "How to test" section in this PR
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request adds the role='button' attribute to the search item in the main navigation drawer, improving accessibility for screen readers and assistive technologies.

  • Added role='button' prop to search item in packages/twenty-front/src/modules/navigation/components/MainNavigationDrawerItems.tsx
  • Implemented 'role' prop in packages/twenty-front/src/modules/ui/navigation/navigation-drawer/components/NavigationDrawerItem.tsx
  • Applied role attribute to StyledItem component in NavigationDrawerItem
  • Change addresses issue Add role="button" to navbar search #7575, enhancing navbar search accessibility
  • Modification is minimal and doesn't introduce apparent issues or side effects

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -185,6 +187,7 @@ export const NavigationDrawerItem = ({
as={to ? Link : 'div'}
to={to ? to : undefined}
indentationLevel={indentationLevel}
role={role}
Copy link

Choose a reason for hiding this comment

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

style: Ensure role is only applied when it's a non-empty string

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Thank you a lot for contributing to Twenty! I wrote a comment to discuss the proposed solution with you and my colleagues 😄

@@ -185,6 +187,7 @@ export const NavigationDrawerItem = ({
as={to ? Link : 'div'}
Copy link
Contributor

@Devessier Devessier Oct 11, 2024

Choose a reason for hiding this comment

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

Setting role=button is half a solution to make that component a button. Instead, we should render a <button> HTML tag when we want to display a button. The as property allows us to choose which component we want to render at the end. We should use it to display a button when we want.

Setting an HTML element's role attribute is a last resort. We should always try to use semantic HTML first, and here, using a button element makes sense.

To make it work with the current code, the NavigationDrawerItem component could take an optional as property which could be 'button' | undefined. We could use it to choose which component to render.

@DivyanshuLohani
Copy link
Author

Ok I will work on that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add role="button" to navbar search
4 participants