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: Take the position:absolute back and make sure the v1 and v2 ids are same for new docs #4317

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Feb 28, 2024

position:absolute on the tooltip element was introduced to in #4250 to fix an issue on the Tooltip V2 react docs (tooltip is not visible) but it ended up causing a problem at dotcom (specifically memex), flickering and shifting when the tooltip appears. Without this change, everything except the docs is working fine. I think the issue is not on the tooltip but on docs, maybe? I'd like to take this change back to unblock the version upgrade and can look into further later. Though, all react docs is going to be migrated over to the new docs and the way that examples are rendered differently (from stories) on the new site so I'll see if the same issue persistent on the new docs first.

Changelog

New

Changed

  • Updated the tooltip v2's id to be the same as v1 for consolidation on the new docs site

Removed

  • Removed position: absolute from the tooltip element.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

  • Can view the stories and make sure the tooltip appears correctly
  • Unfortunately, (as far as I know) there is no way of testing docs changes in the new site until the release is out

Merge checklist

Copy link

changeset-bot bot commented Feb 28, 2024

🦋 Changeset detected

Latest commit: f08e78b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Feb 28, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 113.49 KB (+0.01% 🔺)
packages/react/dist/browser.umd.js 114.17 KB (-0.01% 🔽)

@@ -23,7 +23,6 @@ const StyledTooltip = styled.div`
/* Overriding the default popover styles */
display: none;
&[popover] {
position: absolute;
Copy link
Member Author

Choose a reason for hiding this comment

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

This change was introduced to in #4250 to fix an issue on the Tooltip V2 react docs but it ended up causing a problem at dotcom (specifically memex), flickering and shifting when the tooltip appears. Without this line, everything except the docs is working fine. I think the issue is not on the tooltip but on docs, maybe? I'd like to take this change back to unblock the version upgrade and can look into further later. Though, all react docs is going to be migrated over to the new docs and the way that examples are rendered differently (from stories) on the new site so I'll see if the same issue persistent on the new docs first.

Copy link
Member Author

Choose a reason for hiding this comment

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

The docs problem happens because the examples come after the first scroll, so this is why we added position:absolute. However, when I removed the position:absolute the instances at dotcom that come after the the first scroll are still the same. Seems like taking it back will fix things and not introduce any other issues as well.

@@ -1,5 +1,5 @@
---
componentId: tooltip_v2
componentId: tooltip
Copy link
Member Author

Choose a reason for hiding this comment

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

Found out from Cameron that to be able to consolidate the versions of the same component on the new docs site, the ids need to be the same.

@broccolinisoup broccolinisoup changed the title position absolute on the popover-open and make sure the v1 and v2 ids some for new docs Tooltip: Take the position:absolute back and make sure the v1 and v2 ids are same for new docs Feb 28, 2024
@broccolinisoup broccolinisoup added this pull request to the merge queue Feb 28, 2024
Merged via the queue into main with commit 0e744e2 Feb 28, 2024
33 checks passed
@broccolinisoup broccolinisoup deleted the tooltip-position-and-id-fix branch February 28, 2024 21:10
@primer primer bot mentioned this pull request Feb 28, 2024
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
…2 ids are same for new docs (#4317)

* position absolute on the popover-open and make sure the v1 and v2 ids some for new docs

* take position absolute back all together - it is only a docs issue

* add changeset

* update changeset

* couldn't use the new changeset cli - marked the change as major! Take it back it is patch
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.

None yet

3 participants