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

Rework Panel API #1769

Merged
merged 19 commits into from
Jun 26, 2017
Merged

Rework Panel API #1769

merged 19 commits into from
Jun 26, 2017

Conversation

jquense
Copy link
Member

@jquense jquense commented Apr 3, 2016

First pass at fixing #983.

I started with the most verbose version of the API. I really hope we can offer something closer to the current API for simple cases, because this is really verbose. In particular the accordion code feels silly, verbose.

Things I don't like about this currently:

  • needs a less verbose option for common use cases, I want to keep some of the body inference stuff, with the option to drop down to the verbose one.
  • A11y is ugly again, essentially copies the TabContainer
  • panel-title feels dumb, but an extra component feels worse. If there was anyway to guess the right component to use I would just use that...perhaps we just default it to a div and add titleComponentClass?

@taion
Copy link
Member

taion commented Apr 4, 2016

Worth noting, BTW, that .panel is dead in Bootstrap 4.

@jquense
Copy link
Member Author

jquense commented Apr 4, 2016

true tho Card is philosophically similar...I am hoping it lends itself to a better API more tho

@taion
Copy link
Member

taion commented Apr 5, 2016

Can we do something with bsRole here?

I'm thinking we have bsRole of heading, body, footer.

If no bsRole is set for anything, automatically wrap in Panel.Body. If a user doesn't want this behavior, he or she can just set his or her own <CustomPanelBody bsRole="body"> and do whatever in there.

For a <Panel collapse>, automatically wrap the element with bsRole="body" in <Panel.Collapse>.

As for the heading API, maybe <Panel.Title toggle> could automatically set up the Panel.Toggle? Then for users who want the entire heading to toggle can do a <Panel.Heading toggle>. For the accordion case, it'd maybe look like:

<Panel collapse>
  <Panel.Heading>
    <Panel.Title toggle>
      Panel title
    </Panel.Title>
  </Panel.Heading>
  <Panel.Body>
    Panel body
  </Panel.Body>
</Panel>

@jquense
Copy link
Member Author

jquense commented Apr 8, 2016

I'm thinking we have bsRole of heading, body, footer.

that seems like a good idea, though we still need a way to say "don't wrap this table or ListGroup", maybe just have them add bsRole='body'? am I over thinking that?

As for the heading API, maybe <Panel.Title toggle> could automatically set up the Panel.Toggle?

I like that, however there is no good default component for Panel.Title since the <hX> ones are so semantic...maybe just <header>?

@taion
Copy link
Member

taion commented Apr 8, 2016

I do think

If no bsRole is set for anything, automatically wrap in Panel.Body. If a user doesn't want this behavior, he or she can just set his or her own <CustomPanelBody bsRole="body"> and do whatever in there.

is reasonable and will probably be good enough in practice.

That title thing is messy, I dunno.

@jquense
Copy link
Member Author

jquense commented Apr 17, 2016

how do we feel about this APi as it stands? the Title thing is a bit annoying but seems like defaulting to a <div> is the least problematic approach.

I will fix up docs and examples if ya'll feel good about it.

@ctxhou
Copy link

ctxhou commented Apr 25, 2016

The new panel feature will add at next version?

I like this new customized panel title and really need it! 😄

@timkendrick
Copy link

Really nice work on this PR! I've been banging my head against the wall trying to inject an icon and some controls into accordion panel headers, this makes it a piece of cake.

As somebody brand new to react-bootstrap, I personally don't find the API overly verbose – everything does exactly what you'd expect, especially if you're coming from the HTML version of Bootstrap.

Do you know if this is going to make it into master?

@BowlingX
Copy link

BowlingX commented Jun 1, 2016

What's the status for this pull-request? Is there any help needed? Right now you can't customize the toggle element in the header and this looks quite complete :).

@jquense
Copy link
Member Author

jquense commented Jun 27, 2016

I should rebase this...

@taion
Copy link
Member

taion commented Jun 27, 2016

👍

@Siyfion
Copy link

Siyfion commented Jul 29, 2016

@jquense & @taion It's a shame that the Bootstrap API is more flexible than react-bootstrap currently. This is awesome, exactly what is needed in my mind. Any idea of if/when this is likely to get merged in?

@zelias500
Copy link

any update on this?

@jquense
Copy link
Member Author

jquense commented Sep 7, 2016

Anyone want to fix the tests?

@Siyfion
Copy link

Siyfion commented Sep 8, 2016

Wow, good work @jquense but I'm afraid I'm not going to be much help, I don't understand it all well enough. All I know is that I'd like to be able to style my panel headers to make them clickable in their entirety rather than just the heading titles!

@chilijung
Copy link

chilijung commented Sep 18, 2016

@jquense I've been working on the tests! #2217

btw, is this branch is going to merge back to master after the tests is done? Cause I am using this feature in my projects. Thanks!

@Extra-lightwill
Copy link

Extra-lightwill commented Dec 16, 2016

What's the progress on this?

@Laggii
Copy link

Laggii commented Jan 9, 2017

Same question, any updates? I see the panel branch has updated docs just the tests are failing.

@Siyfion
Copy link

Siyfion commented Jan 16, 2017

@jquense Any idea if/when this is going to get merged, the underlying issue is responsible for about 90% of my UI "bugs" list at the moment, it's getting annoying! :/

@jquense
Copy link
Member Author

jquense commented Jan 16, 2017

@Siyfion it will, thanks for the ping on this.

@Siyfion
Copy link

Siyfion commented Jan 16, 2017

@jquense Cheers, like I said, I think this is really my only "main" issue with React-Bootstrap at the moment, so it'd be great to get it all merged in. 👍

@jquense
Copy link
Member Author

jquense commented Jan 16, 2017

this should be goood to go for review @react-bootstrap/collaborators .

Me and @taion don't actually use the Panel components often (one reason why this is been so slow) so any other input is greatly appreciated on the API and on the code quality. @Siyfion if you want to jump in as well and make sure it addresses your use cases and bugs 👍

@ramarivera
Copy link

Awesome!!
Can´t wait for this making it to master!

@jquense
Copy link
Member Author

jquense commented May 17, 2017

done

id="collapsible-panel-example-1"
expanded={this.state.open}
>
<Panel.Collapse>
Copy link
Member

Choose a reason for hiding this comment

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

ping?

<Panel.Heading>
<Panel.Title toggle>Panel heading 1</Panel.Title>
</Panel.Heading>
<Panel.Body collapsible >
Copy link
Member

Choose a reason for hiding this comment

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

ping

@@ -120,9 +120,9 @@ describe('bootstrapUtils', () => {
shouldWarn('expected one of ["minimal","boss","plaid","tweed"]');

const Component = bsStyles(['minimal', 'boss', 'plaid', 'tweed'], 'plaid')(
class extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

this was a bad refactor somewhere, just fixing it

src/Panel.js Outdated
defaultExpanded: PropTypes.bool,
/**
* Controls the collapsed/expanded state ofthe Panel. Requires a `Panel.Collapse` or `<Panel.Body collapsible>` child component
* in order to actually animate out or in,
Copy link
Member

Choose a reason for hiding this comment

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

:p

PanelHeading.defaultProps = defaultProps;
PanelHeading.contextTypes = contextTypes;

export default bsClass('panel', PanelHeading);
Copy link
Member

Choose a reason for hiding this comment

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

arguably this (and the others) should be like <TabPane> where the bsClass is tab-pane

Copy link
Member Author

Choose a reason for hiding this comment

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

lets defer that, we should do do big rethink of this api anyway, and reach some consistent policy for child components

@@ -15,12 +15,18 @@ function curry(fn) {
};
}

export function prefix(props, variant) {
export function prefix(props, context, variant) {
Copy link
Member

Choose a reason for hiding this comment

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

i don't see our using the 3-arg form of this anywhere any more

@taion
Copy link
Member

taion commented Jun 24, 2017

I've been swamped. Will try to look over the weekend.

@jquense jquense merged commit 78d700d into next Jun 26, 2017
@jquense jquense deleted the panel branch June 26, 2017 12:12
jackpetraitis pushed a commit to jackpetraitis/react-bootstrap that referenced this pull request Sep 12, 2017
* Upgrade `uncontrollable` dependency

* remove createClass in tests (react-bootstrap#2586)

* remove createClass in tests

* fix-lint

* update changelog

* Release v0.31.0

* Add toggle button components (react-bootstrap#2252)

I’m tried of rewriting this component all the time :P

* Update Dropdown.js (react-bootstrap#2615)

fix typo

* switch to new one (react-bootstrap#2621)

* Rework Panel API

* update

* fix up the branch, fix examples

* Finish tests

* Update PanelHeading.js

* prop-types clean up

* change collapsible API, finish examples and docs

* lint:

* clean up nits

* nits
@@ -1,5 +1,11 @@
import { cloneElement } from 'react';
import ReactDOM from 'react-dom';
import tsp from 'teaspoon';

tsp.fn.log = function log() {
Copy link
Member

Choose a reason for hiding this comment

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

@jquense Was this addition intentional?

@hitchcockwill
Copy link

What's the latest on this? Anything I can do to help out here?

jquense added a commit that referenced this pull request Jan 2, 2018
* Rework Panel API (#1769)

* Upgrade `uncontrollable` dependency

* remove createClass in tests (#2586)

* remove createClass in tests

* fix-lint

* update changelog

* Release v0.31.0

* Add toggle button components (#2252)

I’m tried of rewriting this component all the time :P

* Update Dropdown.js (#2615)

fix typo

* switch to new one (#2621)

* Rework Panel API

* update

* fix up the branch, fix examples

* Finish tests

* Update PanelHeading.js

* prop-types clean up

* change collapsible API, finish examples and docs

* lint:

* clean up nits

* nits

* remove pagination logic (#2587)

* remove pagination logic

* Add upgrade path.

* Transition group (#2676)

* switch to react transition group

* Update Modal api

* fix tests

* bump and fix test

* Update package.json

* update wording

* preventDefault specified to work only for disabled && onSelect; it is a fix for issue #2711

* removing prevent defaults from any onSelect logic for NavItem, Panel, and PanelGroup

* adding tests for Panel and NavItem changes. testing for whether preventDefault is clicked and for whether the navbar collapses when it is clicked.

* part of the fix for #2711, removing preventDefault for onSelect. also modifying code so it only will preventDefault if element is disabled

* refactoring the fix for issue #2711

* figured this test was unnecessary. also fixed some linting errors. for #2711

* merge issues

* fix lint
jquense pushed a commit that referenced this pull request Feb 8, 2018
* Rework Panel API (#1769)

* Upgrade `uncontrollable` dependency

* remove createClass in tests (#2586)

* remove createClass in tests

* fix-lint

* update changelog

* Release v0.31.0

* Add toggle button components (#2252)

I’m tried of rewriting this component all the time :P

* Update Dropdown.js (#2615)

fix typo

* switch to new one (#2621)

* Rework Panel API

* update

* fix up the branch, fix examples

* Finish tests

* Update PanelHeading.js

* prop-types clean up

* change collapsible API, finish examples and docs

* lint:

* clean up nits

* nits

* remove pagination logic (#2587)

* remove pagination logic

* Add upgrade path.

* Transition group (#2676)

* switch to react transition group

* Update Modal api

* fix tests

* bump and fix test

* Update package.json

* update wording

* preventDefault specified to work only for disabled && onSelect; it is a fix for issue #2711

* removing prevent defaults from any onSelect logic for NavItem, Panel, and PanelGroup

* adding tests for Panel and NavItem changes. testing for whether preventDefault is clicked and for whether the navbar collapses when it is clicked.

* part of the fix for #2711, removing preventDefault for onSelect. also modifying code so it only will preventDefault if element is disabled

* refactoring the fix for issue #2711

* figured this test was unnecessary. also fixed some linting errors. for #2711

* merge issues

* fix lint

* Add up-to-date Bootstrap stylesheets

* Add btn-link to storybook, precommit format

* Fix toggle buttons

* Update toggle prop on ButtonGroup

* Use npm registry
jquense pushed a commit that referenced this pull request Feb 26, 2018
* Rework Panel API (#1769)

* Upgrade `uncontrollable` dependency

* remove createClass in tests (#2586)

* remove createClass in tests

* fix-lint

* update changelog

* Release v0.31.0

* Add toggle button components (#2252)

I’m tried of rewriting this component all the time :P

* Update Dropdown.js (#2615)

fix typo

* switch to new one (#2621)

* Rework Panel API

* update

* fix up the branch, fix examples

* Finish tests

* Update PanelHeading.js

* prop-types clean up

* change collapsible API, finish examples and docs

* lint:

* clean up nits

* nits

* remove pagination logic (#2587)

* remove pagination logic

* Add upgrade path.

* Transition group (#2676)

* switch to react transition group

* Update Modal api

* fix tests

* bump and fix test

* Update package.json

* update wording

* preventDefault specified to work only for disabled && onSelect; it is a fix for issue #2711

* removing prevent defaults from any onSelect logic for NavItem, Panel, and PanelGroup

* adding tests for Panel and NavItem changes. testing for whether preventDefault is clicked and for whether the navbar collapses when it is clicked.

* part of the fix for #2711, removing preventDefault for onSelect. also modifying code so it only will preventDefault if element is disabled

* refactoring the fix for issue #2711

* figured this test was unnecessary. also fixed some linting errors. for #2711

* merge issues

* fix lint

* Add up-to-date Bootstrap stylesheets

* Add btn-link to storybook, precommit format

* Fix toggle buttons

* Update toggle prop on ButtonGroup

* Use npm registry
@shawnbissell
Copy link

I know I'm late to the game here ... but this was a nasty breaking change. We have over 300 usages of in our app so it looks like we are stuck of version 0.31.5 for the foreseeable future. How are other people upgrading to v0.32.0? Is there a jscodeshift script for this change?

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.