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

Tooltip tail affixation #1477

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Tooltip tail affixation #1477

wants to merge 4 commits into from

Conversation

caseyWebb
Copy link
Member

@caseyWebb caseyWebb commented Aug 22, 2023

🔧 Modifying a component

Context

This PR makes Nri.Tooltip tails always affix to the trigger element. While the API remains the same, this is a breaking change with regards to visual behavior.

The customOffset value passed to alignStart/alignEnd now defines the additional distance to offset the tail from the respective end, rather than the tooltip as a whole.

TODO:

  • components that rely on this and accept additional tooltip attributes should be versioned
  • custom offset should be negated automatically when it makes sense. a value of 100 should push the tail 100px inward (not arbitrarily right), regardless of positioning.

The changes are best demonstrated via video, see below:

🖼️ What does this change look like?

V3 (Current)

V3.mov

V4 (New)

V4.mov

Component completion checklist

  • I've gone through the relevant sections of the Development Accessibility guide with the changes I made to this component in mind
  • Changes are clearly documented
    • Component docs include a changelog
    • Any new exposed functions or properties have docs
  • Changes extend to the Component Catalog
    • The Component Catalog is updated to use the newest version, if appropriate
    • The Component Catalog example version number is updated, if appropriate
    • Any new customizations are available from the Component Catalog
    • The component example still has:
      • an accurate preview
      • valid sample code
      • correct keyboard behavior
      • correct and comprehensive guidance around how to use the component
  • Changes to the component are tested/the new version of the component is tested
  • Component API follows standard patterns in noredink-ui
    • e.g., as a dev, I can conveniently add an nriDescription
    • and adding a new feature to the component will not require major API changes to the component
  • If this is a new major version of the component, our team has stories created to upgrade all instances of the old component. Here are links to the stories:
    • add your story links here OR just write this is not a new major version

@caseyWebb caseyWebb marked this pull request as draft August 22, 2023 03:14
@tesk9
Copy link
Contributor

tesk9 commented Aug 22, 2023

@caseyWebb just to double-check, you're aware that if Kraken makes this change, Kraken will be responsible for updating all tooltip uses to the new version? The upgrade path here seems a little challenging, so I just want to be sure the team has enough bandwidth to get everything done!

@caseyWebb
Copy link
Member Author

caseyWebb commented Aug 24, 2023

Yep, trust me it's a bit daunting 😅. We don't have too many uses of alignStart/alignEnd, but afaict the upgrade path will involve checking each one so definitely thank you for raising early.

The upside is this isn't terribly high priority, and I can work asynchronously to upgrade existing ones before needing to merge this, and if it truly is a complete headache this isn't strictly necessary, just very nice to have. The other slight saving grace is that the custom offset is most likely (I hope) accomplishing what this PR does automatically, so should generally involve setting it to zero (I think).

@tesk9
Copy link
Contributor

tesk9 commented Aug 30, 2023

If you do end up going for it, I recommend testing your changes with Nri.Writing.UITour as early as you can. It's a weird/non-standard use of tooltips (and a pattern that shouldn't be replicated anywhere else), but it's the area where I think this change might be particularly tricky. I could be wrong, but that's my guess!

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.

2 participants