Add fixed position prop to Overlay

[ajhenry]

This adds the following CSS properties as props to the Overlay prop to customize its position behavior.

position
top (modified)
left (modified)
right
bottom

I added tests and a story to demonstrate the new behavior.

Closes #2713

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 0ef3178

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	Minor

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

[colebemis]

@ajhenry Thank you for opening up this PR!

How do you feel about this prop API instead:

+ position: 'absolute' | 'fixed' (default: 'absolute')
  top?: number
  left?: number
+ right?: number (default: undefined)
+ bottom?: number (default: undefined)

If it feels a little more consistent with CSS.

[ajhenry]

@colebemis That makes a lot of sense to me!

Looking at it, it would probably make the most sense just to remove the left prop altogether as position and left|right|top|bottom will be added via the sx prop that's on the object

i.e. this is already valid today

<Overlay
  position="fixed"
  right={0}
/>

[ajhenry]

@colebemis I can revise this PR today to make those changes and see how we like them!

[ajhenry]

@colebemis friendly bump to see if this is what you're looking for 😄

[lesliecdubs]

Hi @ajhenry, thanks for working on this! @colebemis is out of office this week.

Will it block you if he gets back to you on/around January 17 when he's back? If so, let me know and I can loop in another team member to review.

[ajhenry]

@lesliecdubs No problem, thanks for the heads up! This isn't blocking anything so we're good

[colebemis]

These changes look great. That is exactly what I had in mind when I wrote my previous comment 👍

One thing before I approve and merge the PR: Can you update the docs to match your updated code?

[ajhenry]

@colebemis Updated the docs! Let me know if I need to do anything else 🚀

[colebemis]

This failing test looks related to these changes:

https://github.com/primer/react/actions/runs/3970131563/jobs/6805498589#step:8:606

[ajhenry]

Taking a look now

[ajhenry]

So this is failing because of the way we try to parse coordinates for the AutoSuggestions component.

The following calculation to determine line height is returning NaN for specific cases.

react/src/drafts/utils/character-coordinates.ts

Lines 80 to 82 in 4dcf658

	const lineHeight = isNaN(parseInt(computed.lineHeight))
	? parseInt(computed.fontSize) * 1.2
	: parseInt(computed.lineHeight)

When computed.fontSize is normal we fallback to computed.fontSize which can be blank. The problem with this is that parsing a blank string results in NaN.

parseInt("")
-> NaN

Should we default to 1 when fontSize is blank?

For example, when changing that util function to be the following, we no longer have a failing test

// Lineheight is either a number or the string 'normal'. In that case, fall back to a
// rough guess of 1.2 based on MDN: "Desktop browsers use a default value of roughly 1.2".
const lineHeight = isNaN(parseInt(computed.lineHeight))
  ? parseInt(computed.fontSize === '' ? '1' : computed.fontSize) * 1.2
  : parseInt(computed.lineHeight)

[colebemis]

Did some digging and this should fix the failing test:

diff --git a/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx b/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx
index d29b5e122..53e64d282 100644
--- a/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx
+++ b/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx
@@ -198,8 +198,8 @@ const InlineAutocomplete = ({
         inputRef={inputRef}
         onCommit={onCommit}
         onClose={onHideSuggestions}
-        top={suggestionsOffset.top}
-        left={suggestionsOffset.left}
+        top={suggestionsOffset.top || 0}
+        left={suggestionsOffset.left || 0}
         visible={suggestionsVisible}
         tabInsertsSuggestions={tabInsertsSuggestions}
       />

The NaN was probably an issue before this PR, but the previous implementation covered up the issue by doing top: ${top || 0}px`.

[ajhenry]

I think this is ready for action @colebemis

[colebemis]

🥳