Skip to content

USWDS - Tooltip: Add top as default position#5228

Merged
thisisdano merged 1 commit into
uswds:developfrom
danbrady:db-fix-tooltip-position
May 26, 2023
Merged

USWDS - Tooltip: Add top as default position#5228
thisisdano merged 1 commit into
uswds:developfrom
danbrady:db-fix-tooltip-position

Conversation

@danbrady

@danbrady danbrady commented Apr 10, 2023

Copy link
Copy Markdown
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.

mejiaj
mejiaj approved these changes Apr 14, 2023
View reviewed changes

@mejiaj mejiaj left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

mejiaj
mejiaj reviewed Apr 14, 2023
View reviewed changes
Comment thread packages/usa-tooltip/src/index.js Outdated

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

@mejiaj mejiaj Apr 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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") => {

@danbrady danbrady Apr 15, 2023

Copy link
Copy Markdown
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
mahoneycm
mahoneycm approved these changes Apr 17, 2023
View reviewed changes

@mahoneycm mahoneycm left a comment

Copy link
Copy Markdown
Contributor

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 👍

amyleadem
amyleadem reviewed Apr 17, 2023
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

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

mejiaj commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

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

danbrady commented Apr 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

mejiaj commented Apr 21, 2023

Copy link
Copy Markdown
Contributor

@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

danbrady commented Apr 25, 2023

Copy link
Copy Markdown
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
amyleadem
amyleadem approved these changes Apr 26, 2023
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

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

mejiaj
mejiaj approved these changes May 24, 2023
View reviewed changes

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

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

thisisdano commented May 26, 2023

Copy link
Copy Markdown
Contributor

Thank you!

@thisisdano thisisdano merged commit d9ff9f8 into uswds:develop May 26, 2023
This was referenced May 31, 2023
Merged
Merged
@thisisdano thisisdano mentioned this pull request Jun 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers
@mejiaj mejiaj mejiaj approved these changes
@thisisdano thisisdano thisisdano approved these changes
@mahoneycm mahoneycm mahoneycm approved these changes
@amyleadem amyleadem amyleadem approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Tooltip documentation claims the tooltip positioned "top" when not specified; Incorrectly positions bottom left

5 participants

@danbrady @mejiaj @thisisdano @mahoneycm @amyleadem