-
Notifications
You must be signed in to change notification settings - Fork 358
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(Pagination): add disabled flag for whole component #2586
Conversation
PatternFly-React preview: https://patternfly-react-pr-2586.surge.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing a class? |
0e8bb9e
to
1a0f09e
Compare
1a0f09e
to
7f67e2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Can you update the demo app and the integration test please
b079f62
to
44bec10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
parentRef = null, | ||
toggleTemplate: ToggleTemplate = '', | ||
}:OptionsToggleProps ) => { | ||
return ( | ||
<div className={css(styles.optionsMenuToggle, getModifier(styles, 'plain'), getModifier(styles, 'text'))} > | ||
<div className={css(styles.optionsMenuToggle, isDisabled && getModifier(styles, 'disabled'), getModifier(styles, 'plain'), getModifier(styles, 'text'))} > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it generally makes more sense to use getModifier if a variable prop can be matched to the style object. In this case i would change these to styles.disabled
, styles.plain
, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I thought that was just how we retrieved modifier styles. I can change it to styles.modifiers.disabled
and so on.
import React from 'react'; | ||
import { Pagination, PaginationVariant } from '@patternfly/react-core'; | ||
|
||
class PaginationTop extends React.Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: class name should be PaginationDisabled
. Doesn't affect the rendering but shows up in the sample code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Adds 'isDisabled' flag to disable the whole pagination component.
See issue: #2315