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

[changed] Reworks TabbedArea and TabPane API #1091

Closed
wants to merge 8 commits into from

Conversation

kennyw12
Copy link

Renames TabbedArea to Tabs
Renames TabPane to Tab
Renames tab prop on Tab to title
className on Tab only applies to .tab-pane
className, id, and style on Tabs applies to outermost div
Removes BootstrapMixin from Tabs to remove faulty documentation.
namely: bsSize and unsupported bsStyle options
#1054

@taion
Copy link
Member

taion commented Jul 31, 2015

Taking a very, very cursory glance, but it looks like this doesn't deprecate the components under the old names, but instead removes the old ones completely?

@jquense
Copy link
Member

jquense commented Jul 31, 2015

This is super cool 👍 I haven't had a chance to dig in but I like the changes. As @taion tho we need to talk about a deprecation path for big changes like this where they are possible (the smaller className changes probably aren't deprecatable). Luckily it doesn't look like it will be too difficult in these instances :)

@AlexKVal
Copy link
Member

Looks nice 👍 ❇️

But it should be pull requested against v0.26-rc and added to v0.26 Release Milestone.

And an additional PR with deprecations should be pull requested against v0.25-rc.

Like this: factories deprecation
#1019 deprecation messages => v0.24
#1020 removing of factories => v0.25

🍒

@timv88
Copy link

timv88 commented Aug 1, 2015

+1 for adding className to the most outer element of the tabs. I think this should be a standard for every react-bootstrap component.

Currently I have to work around this by having the following CSS selector > div:not([class]):not([id]) {...}. Not cool.

@kennyw12
Copy link
Author

kennyw12 commented Aug 3, 2015

Hi, I don't see a v0.26-rc tag or branch on react-bootstrap/react-bootstrap. I also don't quite know what you mean by adding something to a Milestone. How do I do that?

I'm also not sure if I added deprecation warnings correctly in #1099.
Sorry.

@jquense
Copy link
Member

jquense commented Aug 4, 2015

@kennyw12 what I would do is submit this PR against the next major version branch: v0.25-rc and within that PR also provide aliased components under the old names that warn that they are deprecated. So if a user uses TabPane they will get a warning but it will render the now Tab component. That way users can switch to the new components slowly (over the course the 25 version anyway).

I probably would also split the commits up into maybe two commits for the sake of the change log, one for renaming components, and the other documenting the className shifts, since that is also a breaking change.

@kennyw12
Copy link
Author

kennyw12 commented Aug 4, 2015

Just to clarify, sounds like we should have 3 commits:

  1. Deprecates TabbedArea and TabPane(aliasing them to the new Tabs and Tab), and adds in Tabs and Tab (including both sets of tests and docs, the old docs marked with a deprecation warning). This is against v0.25-rc
  2. Removes TabbedArea and TabPane, and all associated docs and tests. This is against v0.26-rc, which still needs to be created as a tag
  3. Make the className shifts against 0.26-rc, on top of Draft component designs #2. (This change seems like it would not be able to be deprecated since it changes where classNames go.)

Would that work for you? If so, could you make the 0.26-rc tag?
Thanks!

The Pivotal UI Team
@atomanyih @ctaymor @gpleiss @kennyw12 @matt-royal @nicw @stubbornella

@jquense
Copy link
Member

jquense commented Aug 4, 2015

There is no need to put the className changes in 0.26 not every breaking change needs a deprecation, since 0.25 is the next "major" version its fine to have breaking changes in it. The deprecation policy is just: "when a deprecation is possible, the features will be deprecated for a full major version before removed",

I will look into a 0.26-rc branch, sorry our actual process is lagging behind the new deprecation policy.

Caroline Taymor and others added 2 commits August 4, 2015 15:30
className, id, and style props for TabbedArea are passed to the outer
div.

Signed-off-by: Kenny Wang <kwang@pivotal.io>
* Deprecates TabbedArea and TabPane
* Adds Tabs (formerly TabbedArea) and Tab (formerly TabPane)
* `tab` attribute is renamed to `title`
* Removes tests for TabbedArea and TabPane (because the deprecation
warning fails them all
* Update docs to use Tabs and Tab examples

Signed-off-by: Caroline Taymor <ctaymor@pivotal.io>
@kennyw12
Copy link
Author

kennyw12 commented Aug 5, 2015

When a branch for 0.26-rc is created, we will create a second PR against it to remove TabbedArea and TabPane.

The Pivotal UI Team
@atomanyih @ctaymor @gpleiss @kennyw12 @matt-royal @nicw @stubbornella

@jquense
Copy link
Member

jquense commented Aug 5, 2015

@kennyw12 hey there, this PR is still against master you'll have to close this probably and re-PR against the 0.25 branch. Sorry for all the rigamarole, we appreciate the help :)

@AlexKVal AlexKVal added this to the v0.26 Release milestone Aug 5, 2015
@AlexKVal
Copy link
Member

AlexKVal commented Aug 5, 2015

I like the proposal #1091 (comment) of @jquense 👍
@mtscout6 points to about the same here #1099 (comment)

Then you won't need to make any additional PRs against v0.26-rc. ❇️
(i.e. this #1099 is not needed then.)

And react-bootstrap's team will remove deprecated code in the future with v0.26-rc version.
🍒

@AlexKVal
Copy link
Member

AlexKVal commented Aug 5, 2015

this is now like this:
screen shot 2015-08-05 at 11 02 14 am
and it should be re-based like this:
screen shot 2015-08-05 at 11 03 01 am

This is the way as I would doing it:

> git remote -v
origin ...
upstream    https://github.com/react-bootstrap/react-bootstrap.git (fetch)
upstream    https://github.com/react-bootstrap/react-bootstrap.git (push)
> git checkout master
> git fetch upstream
> git checkout v0.25-rc
> git branch tabs-rework
> git checkout tabs-rework

then you make your changes and PR them.
And on the GitHub site then you choose branch against which you want merge it: react-bootstra/v0.25-rc

I hope it will help 🍒

this PR o/c will be closed and the new one will be created, as @jquense points out #1091 (comment)

@kennyw12 kennyw12 mentioned this pull request Aug 5, 2015
@kennyw12
Copy link
Author

kennyw12 commented Aug 5, 2015

Alright, we made the same PR against v0.25-rc (#1109). Closing this PR.

Thanks!

@kennyw12 kennyw12 closed this Aug 5, 2015
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.

None yet

6 participants