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

USWDS - Tooltip: Add top as default position #5228

Merged
merged 1 commit into from
May 26, 2023

Conversation

danbrady
Copy link
Contributor

Summary

Added default positioning. Updated the default position to "top" if data-position attribute is not specified.

Breaking change

This is not a breaking change.

Related issue

Closes #5218

Problem statement

  1. Tooltip should appear on "top" when data-position omitted
  2. Tooltip appears incorrectly positioned (at bottom left, with triangle pointing down)
  3. If data-position is not specified, tooltip appears on broken

Solution

  1. Detect if data-position on trigger is set
  2. Use set position or "top" if not

Testing and review

  1. Used this markup to test: <button type="button" class="usa-button usa-tooltip" title="Show me correctly">Show on top</button>
  2. I didn't add this variation into the examples, but let me know if that is desired.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @danbrady! I tested the following markup:

<button type="button" class="usa-button usa-tooltip"  title="No position set on this tooltip">No position set</button>

And was able to get the expected behavior.

image

@@ -369,7 +369,10 @@ const tooltip = behavior(
[TOOLTIP_TRIGGER](e) {
const { trigger, body } = getTooltipElements(e.target);

showToolTip(body, trigger, trigger.dataset.position);
// Default to "top" if `data-position` not specified
const position = trigger.dataset.position || "top";
Copy link
Contributor

@mejiaj mejiaj Apr 14, 2023

Choose a reason for hiding this comment

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

Adding additional note as I remembered this was supposed to be handled in :318. We check for position in setUpAttributes and use the default.


Alternatively, we could pass it as a default parameter in showTooltip:

const showToolTip = (tooltipBody, tooltipTrigger, position="top") => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that's a better solution. Updated the PR. Thanks @mejiaj!

@danbrady danbrady force-pushed the db-fix-tooltip-position branch from b43377a to 68cd518 Compare April 15, 2023 23:48
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Love having this as a fallback! Working great on my local 👍

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Thanks for this submission @danbrady! I confirmed that the tooltip positioning now works as expected when no data-position attribute is added in the markup.

I just had one question:
@mejiaj - I noticed you flagged line 318 (now 320). Is the top assignment there still needed?

@mejiaj
Copy link
Contributor

mejiaj commented Apr 19, 2023

I just had one question: @mejiaj - I noticed you flagged line 318 (now 320). Is the top assignment there still needed?

Yes, but it could be improved by setting the data-position to top via setAttribute() or dataset.position = "top". Currently, it won't do anything if we default to string "top".

@danbrady
Copy link
Contributor Author

danbrady commented Apr 19, 2023

FWIW, it appears that removing lines 318-320 as well as 'position' from the returning object on 355 will have no impact to the functionality of the tooltip, even before this PR. The positioning happens when showTooltip is called and it passes the value from trigger.dataset.position on 374.

I can update the PR to remove these line and keep the position param with a default value of 'top'. Or, alternatively, update the setUpAttributes method to do the position detection or fallback to 'top' (via setAttribute) and remove the default position param value. Is either one of those preferred?

@mejiaj
Copy link
Contributor

mejiaj commented Apr 21, 2023

@danbrady removing 318-320 might not have any effect on the functionality, but setting the attribute would be helpful in making the experience consistent and possibly easier to debug.

@danbrady danbrady force-pushed the db-fix-tooltip-position branch 3 times, most recently from ae10638 to 1c4446b Compare April 24, 2023 20:21
@danbrady danbrady force-pushed the db-fix-tooltip-position branch from 1c4446b to 513c74d Compare April 25, 2023 11:32
@danbrady
Copy link
Contributor Author

@mejiaj Ok, updated! Let me know if that's more inline. I kept all the same logic in place for consistency, but safely added default "top" positioning.

@danbrady danbrady requested a review from mejiaj April 25, 2023 11:39
@amyleadem amyleadem self-requested a review April 26, 2023 16:31
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

I like this solution. It works well!

I tested this tooltip html with no data-position attribute:

<button type="button" class="usa-button usa-tooltip" title="No position set on this tooltip">No data-position set</button>

And I saw that the tooltip is successfully assigning top position:
image

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Nice fix, thank you!

Tested

<div class="margin-4">
    <button type="button" class="usa-button usa-tooltip" title="Forgot to position">No position set</button>
</div>

Result
image

@mejiaj mejiaj requested a review from thisisdano May 24, 2023 19:33
@thisisdano
Copy link
Member

Thank you!

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