Skip to content

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

Merged
merged 10 commits into from
Jul 3, 2024
Merged

defaultVisible prop #7255

merged 10 commits into from
Jul 3, 2024

Conversation

jpranays
Copy link
Contributor

@jpranays jpranays commented Jun 17, 2024

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.
  • The correct query is used. (Refer to this list of queries)
  • 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

@britt6612
Copy link
Collaborator

@jpranays anyway you can add a test for this ?

@britt6612
Copy link
Collaborator

@jpranays do you also have a use case for this desired behavior?

@jpranays
Copy link
Contributor Author

@jpranays anyway you can add a test for this ?
sure

@jpranays
Copy link
Contributor Author

@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.

@jpranays
Copy link
Contributor Author

Any update on this ?

@jcfilben
Copy link
Collaborator

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.

@jpranays
Copy link
Contributor Author

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.

@jcfilben
Copy link
Collaborator

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

@jcfilben
Copy link
Collaborator

Thanks for sharing, we will take a look at these

@jcfilben
Copy link
Collaborator

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.

@jpranays
Copy link
Contributor Author

Thanks for letting me know.

@jcfilben
Copy link
Collaborator

Hey @jpranays after talking through the grommet team is supportive of this enhancement. Thanks again for providing links and detailed responses to our questions.

Copy link
Collaborator

@jcfilben jcfilben left a 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?

@jpranays
Copy link
Contributor Author

That’s great to hear!
Welcome!!

Copy link
Collaborator

@jcfilben jcfilben 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!

@jcfilben jcfilben merged commit 0e74247 into grommet:master Jul 3, 2024
abdulbasithqb pushed a commit to qburst/grommet-1 that referenced this pull request Jul 11, 2024
* 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>
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