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

fix(ui-popover): add scrollbar to Popover if it is out of view #1885

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joyenjoyer
Copy link
Contributor

@joyenjoyer joyenjoyer commented Mar 4, 2025

INSTUI-4453

Test 1

  1. Create a Popover with content enough to go offscreen if you Zoom in 400%
  2. Zoom in 400%
  3. Check if a scrollbar gets displayed

Test 2

  1. Add a Popover inside a Modal (trigger button should be in a Modal) with content enough to go offscreen if you Zoom in 400%
  2. Zoom in 400%
  3. Check if a scrollbar gets displayed

The Popover being slightly off screen during zoom is not an issue according to Reka.

@joyenjoyer joyenjoyer self-assigned this Mar 4, 2025
Copy link

github-actions bot commented Mar 4, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://instructure.github.io/instructure-ui/pr-preview/pr-1885/

Built to branch gh-pages at 2025-03-04 10:02 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@joyenjoyer joyenjoyer requested review from matyasf and balzss March 4, 2025 10:05
@joyenjoyer joyenjoyer changed the title fix(ui-popover): add scrollbar to popover if it out of view fix(ui-popover): add scrollbar to popover if it is out of view Mar 4, 2025
if (this._contentElement) {
const rect = this._contentElement.getBoundingClientRect()
const windowHeight = window.innerHeight
return rect.bottom > windowHeight || rect.top < 0
Copy link
Contributor Author

@joyenjoyer joyenjoyer Mar 4, 2025

Choose a reason for hiding this comment

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

I am using the ref for the displayed content and check whether it crosses the top or bottom side of the browser window.

@@ -272,6 +281,10 @@ class Popover extends Component<PopoverProps, PopoverState> {
}
}

if (this.isContentOutOfView()) {
offsetY = 0
Copy link
Contributor Author

@joyenjoyer joyenjoyer Mar 4, 2025

Choose a reason for hiding this comment

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

If part of the displayed content is offscreen, setting an offset for Y is pointless and it breaks the logic used in line 594.

@@ -578,7 +591,15 @@ class Popover extends Component<PopoverProps, PopoverState> {

return (
<ContextView {...viewProps} borderColor={styles?.borderColor}>
{content}
<div
style={{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this here instead of putting into the styles file because 1/ it contains logic, 2/ this is not purely styling.

@joyenjoyer joyenjoyer changed the title fix(ui-popover): add scrollbar to popover if it is out of view fix(ui-popover): add scrollbar to Popover if it is out of view Mar 4, 2025
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

If I use the test code below it first adds a scrollbar, but if I close and open the popover again it does not

class Example extends React.Component {
  state = {
    isShowingContent: false
  }

  renderCloseButton() {
    return (
      <CloseButton
        placement="end"
        offset="small"
        onClick={() => this.setState({ isShowingContent: false })}
        screenReaderLabel="Close"
      />
    )
  }

  render() {
    return (
      <View>
        <Popover
          renderTrigger={<Button>Sign In</Button>}
          isShowingContent={this.state.isShowingContent}
          onShowContent={(e) => {
            this.setState({ isShowingContent: true })
          }}
          onHideContent={(e, { documentClick }) => {
            this.setState({ isShowingContent: false })
          }}
          on="click"
          screenReaderLabel="Popover Dialog Example"
          shouldContainFocus
          shouldReturnFocus
          shouldCloseOnDocumentClick
          offsetY="16px"
        >
          <View padding="medium" display="block" as="form">
            {this.renderCloseButton()}
            <FormFieldGroup description="Log In">
              <TextInput
                renderLabel="Username"
                inputRef={(el) => {
                  if (el) {
                    this._username = el
                  }
                }}
              />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
              <TextInput renderLabel="Password" type="password" />
            </FormFieldGroup>
          </View>
        </Popover>
      </View>
    )
  }
}

render(<Example />)

isContentOutOfView() {
if (this._contentElement) {
const rect = this._contentElement.getBoundingClientRect()
const windowHeight = window.innerHeight
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think trying to access window with SSR will lead to a crash? Can you please use canUseDOM and if its false this function could just return false.

@balzss balzss removed their request for review March 6, 2025 12:44
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