-
Notifications
You must be signed in to change notification settings - Fork 1k
defaultVisible prop #7255
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
defaultVisible prop #7255
Conversation
@jpranays anyway you can add a test for this ? |
@jpranays do you also have a use case for this desired behavior? |
|
No as such of special use case, but we've had one backend api based requirement, where props were passed through the api response, but no matching mapping was found for defaultVisible prop. Making tooltips visible by default can greatly improve a website's accessibility, usability, and overall user experience. It ensures that all users, no matter their interaction methods or abilities, have immediate access to essential information, thereby making the interface more intuitive and inclusive. |
Any update on this ? |
Hey @jpranays thanks for working on this and sharing your ideas with us. A few thoughts: You mentioned that part of the desire for adding this is to improve accessibility, usability, and overall experience. However, having the tooltip visible by default seems to go against the W3C pattern for how a tooltip should behave: Currently our Tip component is accessible via keyboard focus and mouse hover which aligns with the accessibility guidance we have seen. You also mentioned that having the tooltip visible by default will enable 'immediate access to essential information, thereby making the interface more intuitive and inclusive'. I think if information is essential it should not be displayed in a tooltip and a different component should be used so that the information is always visible. |
Thank you for your thoughtful feedback and your commitment to accessibility and adherence to the W3C guidelines for tooltips. The current implementation of the Tip component aligns well with these guidelines, ensuring both accessibility and user-friendliness. However, I don’t believe that having a defaultVisible prop would violate W3C guidelines. On the contrary, it would enhance the component’s accessibility. Additionally, other major libraries include a defaultVisible or defaultOpen prop, further extending the component’s accessibility. |
Could you share what other libraries are supporting this? I think that would be helpful for us to looks at as we evaluate this enhancement |
Thanks for sharing, we will take a look at these |
The Grommet team is planning to discuss this a bit more on Thursday, I'll keep you in the loop with any updates and post them here. |
Thanks for letting me know. |
Hey @jpranays after talking through the grommet team is supportive of this enhancement. Thanks again for providing links and detailed responses to our questions. |
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.
Can you add defaultVisible
to the Tip/propTypes.js
file?
That’s great to hear! |
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.
Looks good!
* defaultVisible prop interface type entry * Added defaultVisible prop * Added test and its snapshot * Proptype added for defaultVisible * Snapshot updated --------- Co-authored-by: Jadhav <Pranay.Jadhav@transformco.com>
What does this PR do?
Optional defaultVisible prop on Tip component
Where should the reviewer start?
Tip component folder
What testing has been done on this PR?
Tested e2e
How should this be manually tested?
Inside e2e folder, import Tip component in App.js and pass defaultVisible prop
Do Jest tests follow these best practices?
screen
is used for querying.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Do the grommet docs need to be updated?
yes for an additional prop entry
Should this PR be mentioned in the release notes?
Is this change backwards compatible or is it a breaking change?
backwards compatible