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

feat(Popper): add width props, remove popperMatchesTriggerWidth #8724

Merged
merged 13 commits into from Mar 15, 2023

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Feb 20, 2023

What: Closes #8682

Adds width, minWidth, maxWidth to Popper.
Sets the default of minWidth to trigger (popper will grow to trigger width at minimum but will still grow to fit content).
Removes popperMatchesTriggerWidth (minWidth now covers this by default, and all width props can be set to trigger to match the trigger width).

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Feb 20, 2023

Codemod: patternfly/pf-codemods#286

@patternfly-build
Copy link
Contributor

patternfly-build commented Feb 20, 2023

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Feb 22, 2023

Removed several instances of minWidth="revert" that replaced popperMatchesTriggerWidth={false} because the default minWidth will satisfy why the flag was added (as it no longer fixes the width to the trigger).

Added minWidth props to several components to allow customization of the default rather than enforcing a revert or the default. Popover was not included as it has its own minWidth and maxWidth props, and OverflowTab as it is not directly used.

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Noticing some glitchy text on truncated table header text with tooltips.

Mar-03-2023.15-51-03.mp4

Also anywhere we're setting menu widths, we shouldn't need to do that now, unless that's part of the thing we're aiming to show in the example/demo -

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Mar 7, 2023

@mcoker The table tooltip issue you were seeing was resolved in another PR, now that this is rebased it should be fixed here once the build finishes.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Mar 7, 2023

@nicolethoen I'm unsure why this PR is failing a timestamp test as it shouldn't be impacted by these changes. Any thoughts?

@tlabaj
Copy link
Contributor

tlabaj commented Mar 7, 2023

@nicolethoen I'm unsure why this PR is failing a timestamp test as it shouldn't be impacted by these changes. Any thoughts?

Another PR was merges that is now causing this issue. I put up a PR to revert it.

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.

Katie. once you rebase this PR the timestamp test issue should be resolved.

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.

Looks good. just one small suggestion.

packages/react-core/src/next/components/Select/Select.tsx Outdated Show resolved Hide resolved
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.

Can you also remove the minWidth on the DropdownNext kebab example. It should no longer be needed.

I am actually wondering if we need the minWidth prop anymore. It was specifically added for the kebab use case. Menu does not expose a minWidth prop.

cc @mcoker

@tlabaj tlabaj requested a review from srambach March 10, 2023 16:18
@@ -1,7 +1,7 @@
import React from 'react';
import { css } from '@patternfly/react-styles';
import { Menu, MenuContent, MenuProps } from '../../../components/Menu';
import { Popper } from '../../../helpers/Popper/Popper';
import { Popper, PopperProps } from '../../../helpers/Popper/Popper';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think the Select still needs a minwidth prop?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Select (and Dropdown) still have access to the minWidth prop via popperProps that get spread directly to Popper.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just wondering if it would just be spread with popperProps. For The next Dropdown it looks like minWidth was removed on line 26. We should probably be consistent wit the two components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah forgot about that. I'll remove it from Select as well

Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

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

Looks ok to me 👍

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.

LGTM

@tlabaj tlabaj merged commit ec65022 into patternfly:v5 Mar 15, 2023
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-code-editor@5.0.0-alpha.34
  • @patternfly/react-core@5.0.0-alpha.34
  • @patternfly/react-docs@6.0.0-alpha.37
  • demo-app-ts@5.0.0-alpha.17
  • @patternfly/react-table@5.0.0-alpha.34

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popper - allow custom width prop
7 participants