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(chip group): add prop to allow chip group to be initially expanded #2651

Merged
merged 7 commits into from Aug 19, 2019

Conversation

@jenny-s51
Copy link
Contributor

jenny-s51 commented Aug 6, 2019

What: Closes #1697.

@jenny-s51 jenny-s51 requested a review from jcaianirh Aug 6, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 6, 2019

PatternFly-React preview: https://patternfly-react-pr-2651.surge.sh

Copy link
Contributor

jcaianirh left a comment

lgtm thanks @jenny-s51

@@ -11,6 +11,8 @@ export interface ChipGroupProps extends React.HTMLProps<HTMLDivElement> {
children?: React.ReactNode;
/** Additional classes added to the chip item */
className?: string;
/** Flag for having the chip group default to expanded */
isOpenOnInit?: boolean;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Aug 7, 2019

Contributor

Hi. I was just talking to @jschuler about the name of this prop. We want to be consistent with the way we name our props across components. The Page has a similar prop called 'defaultManagedSidebarIsOpen' so we think the chip Group should have a similar name. How about 'defaultIsOpen`. I will also rename the Page prop to defaultManagedSidebarIsOpen for consistency.

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Aug 7, 2019

Author Contributor

Sounds good, thank you Titani -- I'll rename it accordingly

@jenny-s51 jenny-s51 dismissed stale reviews from jcaianirh and kmcfaul via 284df7e Aug 7, 2019
@jenny-s51 jenny-s51 requested review from tlabaj, jcaianirh and kmcfaul Aug 7, 2019
Copy link
Contributor

tlabaj left a comment

Looking good so far. Can you add an example to the integration app and add a cypress test for this as well. Thanks.

@jenny-s51

This comment has been minimized.

Copy link
Contributor Author

jenny-s51 commented Aug 7, 2019

@tlabaj absolutely!

@jenny-s51 jenny-s51 requested a review from tlabaj Aug 8, 2019
Copy link
Contributor

jcaianirh left a comment

lgtm

@jenny-s51 jenny-s51 requested review from tlabaj and jcaianirh Aug 8, 2019
cy.url().should('eq', 'http://localhost:3000/chipgroup-default-is-open-demo-nav-link');
});

it('Verify chip default text', () => {

This comment has been minimized.

Copy link
@tlabaj

tlabaj Aug 9, 2019

Contributor

shouldn't we be verifying the chip group is open?

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Aug 12, 2019

Author Contributor

Yes, nice catch! fixed it 👍

@jenny-s51 jenny-s51 dismissed tlabaj’s stale review Aug 19, 2019

added suggested change

@@ -29,15 +31,16 @@ export class ChipGroup extends React.Component<ChipGroupProps, ChipGroupState>{
constructor(props: ChipGroupProps) {
super(props);
this.state = {
isOpen: false
isOpen: this.props.defaultIsOpen

This comment has been minimized.

Copy link
@redallen

redallen Aug 19, 2019

Contributor

Don't need this., but super(props) does set this.props so you're fine :)

@redallen redallen merged commit 044f98b into patternfly:master Aug 19, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 19, 2019

Your changes have been released in:

  • @patternfly/react-core@3.88.1
  • @patternfly/react-docs@4.10.13
  • @patternfly/react-inline-edit-extension@2.10.9
  • demo-app-ts@2.20.1
  • @patternfly/react-integration@2.20.1
  • @patternfly/react-table@2.18.3
  • @patternfly/react-topology@2.7.34
  • @patternfly/react-virtualized-extension@1.1.120

Thanks for your contribution! 🎉

@jenny-s51 jenny-s51 deleted the jenny-s51:iss1697 branch Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.