Skip to content

fix(Popper): prevented forceUpdate from firing when not necessary#9088

Merged
tlabaj merged 1 commit intopatternfly:v5from
thatblindgeye:iss9012_popperPositioning
May 15, 2023
Merged

fix(Popper): prevented forceUpdate from firing when not necessary#9088
tlabaj merged 1 commit intopatternfly:v5from
thatblindgeye:iss9012_popperPositioning

Conversation

@thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented May 9, 2023 •
edited
Loading

What: Closes #9012

This should resolve the recent visual bug for now. Opened #9089 for investigating an alternate solution.

Additional issues:

@thatblindgeye thatblindgeye requested review from kmcfaul and tlabaj May 9, 2023 13:47
@thatblindgeye thatblindgeye mentioned this pull request May 9, 2023
Closed
@patternfly-build
Copy link
Collaborator

patternfly-build commented May 9, 2023 •
edited
Loading

Preview: https://patternfly-react-pr-9088.surge.sh

A11y report: https://patternfly-react-pr-9088-a11y.surge.sh

tlabaj
tlabaj approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

I am not seeing the behavior anymore. LGTM

nicolethoen
nicolethoen reviewed May 10, 2023
View reviewed changes
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

I am still seeing the behavior the first time this composable advanced search menu is opened. Unless it's a different behavior?
Are there possibly a few things we might need to update in demos like this to make sure it's using your change correctly?

@tlabaj
Copy link
Contributor

tlabaj commented May 10, 2023

I am still seeing the behavior the first time this composable advanced search menu is opened. Unless it's a different behavior? Are there possibly a few things we might need to update in demos like this to make sure it's using your change correctly?

I believe that is a different issue. I will defer to @thatblindgeye.

@thatblindgeye
Copy link
Contributor Author

thatblindgeye commented May 11, 2023

@nicolethoen @tlabaj it doesn't look like it's caused by the forceUpdate call. I added another conditional to only forceUpdate if popper.props.role === 'tooltip' (since that's really the only situation this forceUpdate is for I believe) and the issue still persisted on that search input demo.

@nicolethoen
Copy link
Contributor

nicolethoen commented May 15, 2023

@nicolethoen @tlabaj it doesn't look like it's caused by the forceUpdate call. I added another conditional to only forceUpdate if popper.props.role === 'tooltip' (since that's really the only situation this forceUpdate is for I believe) and the issue still persisted on that search input demo.

I opened an issue to investigate this 'flashing' behavior we are seeing in components/demos using popper.
#9119

@tlabaj tlabaj merged commit a08454d into patternfly:v5 May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolethoen nicolethoen nicolethoen approved these changes

@tlabaj tlabaj tlabaj approved these changes

@kmcfaul kmcfaul kmcfaul approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug - Popper/Dropdown - positioning of popper element is set incorrectly

5 participants

@thatblindgeye @patternfly-build @tlabaj @nicolethoen @kmcfaul