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

Collapsable => collapsible #425

Closed
wmertens opened this issue Mar 1, 2015 · 14 comments · Fixed by #609
Closed

Collapsable => collapsible #425

wmertens opened this issue Mar 1, 2015 · 14 comments · Fixed by #609

Comments

@wmertens
Copy link

wmertens commented Mar 1, 2015

the correct spelling is "collapsible". Perhaps change all occurrences in the code and provide aliases where appropriate?

@mtscout6
Copy link
Member

mtscout6 commented Mar 3, 2015

If we provide an alias than usage of the Alias should output a warning to the console so we can plan to deprecate it later.

@dozoisch
Copy link
Member

Adding that to 1.0.0 wiki

@AlexKVal AlexKVal self-assigned this Apr 27, 2015
@AlexKVal
Copy link
Member

I have already refactored CollapsableMixin => CollapsibleMixin.
All tests are green and dependent components are working with deprecation warnings.

  • CollapsableMixin => CollapsibleMixin
  • refactor CollapsableNav to use the new CollapsibleMixin
  • refactor Panel to use the new CollapsibleMixin
  • refactor ComponentsPage with examples to use the new CollapsibleMixin
  • refactor Nav to use the new CollapsibleMixin
  • CollapsableNav => CollapsibleNav
  • refactor ComponentsPage with examples to use the new CollapsibleNav
  • refactor tests

Deal with

  • CollapsibleNav ref: 'collapsable_' + key

And finally:

  • Document about deprecations

@AlexKVal
Copy link
Member

Rename property collapsable => collapsible for
CollapsableNav, Nav, Panel and PanelGroup components

This is not trivial task and I move it into standalone issue #589,
because it includes components that aren't related
to CollapsableMixin and CollapsableNav.

Nav, Panel and PanelGroup

@AlexKVal
Copy link
Member

Deprecations

Components
CollapsableMixin and CollapsableNav
were moved respectively into
CollapsibleMixin and CollapsibleNav.

CollapsibleMixin has and use four methods.

It has in itself
getCollapsableClassSet() => getCollapsibleClassSet()

It awaits existence of these methods in component
getCollapsableDOMNode()= > getCollapsibleDOMNode()
getCollapsableDimensionValue() => getCollapsibleDimensionValue()
and this one is optional. It can be defined or not defined in component:
getCollapsableDimension() => getCollapsibleDimension()

CollapsibleNav doesn't contain renamed public API methods.

Use of old named components and methods causes warnings about deprecation in console.
Minified version of react-bootstrap does not write any console warnings, because this code are stripped out by webpack DefinePlugin.

These warnings also will be stripped out if you are using envify or webpack DefinePlugin in your project.

AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue Apr 28, 2015
CollapsableMixin => CollaplibleMixin
CollapsableNav   => CollaplibleNav

More here react-bootstrap#425

Exactly this commit:
Add CollapsibleMixin to src/index.js
AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue Apr 28, 2015
CollapsableMixin => CollaplibleMixin
CollapsableNav   => CollaplibleNav

More here react-bootstrap#425

Exactly this commit:
Add CollapsibleMixin to src/index.js
AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue Apr 28, 2015
CollapsableMixin => CollaplibleMixin
CollapsableNav   => CollaplibleNav

More here react-bootstrap#425

Exactly this commit:
Add CollapsibleMixin to src/index.js
Refactor deprecationWarning() for general use.
@AlexKVal
Copy link
Member

What is left:

Rename property collapsable => collapsible for

  • Nav
  • Panel
  • PanelGroup
  • CollapsibleNav
  • Add tests for deprecation warning
  • Make PR

@AlexKVal
Copy link
Member

AlexKVal commented May 1, 2015

This function

function collapsable(props, propName, componentName) {
  if (props[propName] !== undefined) {
    deprecationWarning(
      `${propName} in ${componentName}`,
      'collapsible',
      'https://github.com/react-bootstrap/react-bootstrap/issues/425'
    );
  }
  return React.PropTypes.bool.call(null, props, propName, componentName);
}

is needed to use in CollapsibleNav, Panel and PanelGroup components.
It is about deprecation and will be deleted at some point in future.

I don't want to create another file in utils/_function_for_collapsable_prop_check_.js
(naming should be another of course)

I want to put it into utils/deprecationWarning as

export default function deprecationWarning(...)
...
export function collapsable(props, propName, componentName){...}

and then use it in those three components as:

import deprecationWarning, {collapsable} from './utils/deprecationWarning';

or just

import {collapsable} from './utils/deprecationWarning';

when deprecationWarning() in no need.

Any thoughts ? Suggestions ?

@AlexKVal
Copy link
Member

AlexKVal commented May 1, 2015

Another question:
It seems the collapsable property in PanelGroup is kind of rudiment:
here PanelGroup.js#L13

It was introduced by this patch 855e5b29cb, but it doesn't used anywhere.

This is a property of locally created props object for the child
https://github.com/react-bootstrap/react-bootstrap/blob/master/src/PanelGroup.js#L54

Does this property need deprecation warning or we can just remove it ?
(I've checked: If I remove it then neither tests nor docs are broken)

@mtscout6
Copy link
Member

mtscout6 commented May 1, 2015

I think that one can just be changed to collapsible without a problem. It's being used in the cloneElements call, so it really depends on what's being cloned. But if it's cloning a Panel and if Panel can already handle collapsible than just renaming it should be fine.

AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue May 2, 2015
Discussion is here react-bootstrap#425.

Components are involved:
- Nav
- Panel
- CollapsibleNav

Current property type checking for `collapsable` in `PanelGroup`
is needless and has been removed.
@dozoisch dozoisch added this to the 1.0.0 Release milestone May 2, 2015
AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue May 3, 2015
Discussion is here react-bootstrap#425.

Components are involved:
- Nav
- Panel
- CollapsibleNav

Current property type checking for `collapsable` in `PanelGroup`
is needless and has been removed.
AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue May 3, 2015
Discussion is here react-bootstrap#425.

Components are involved:
- Nav
- Panel
- CollapsibleNav

Current property type checking for `collapsable` in `PanelGroup`
is needless and has been removed.
AlexKVal added a commit to AlexKVal/react-bootstrap that referenced this issue May 4, 2015
Discussion is here react-bootstrap#425.

Components are involved:
- Nav
- Panel
- CollapsibleNav

Current property type checking for `collapsable` in `PanelGroup`
is needless and has been removed.

Tests for deprecated `collapsable` property for all three components
has been placed into one file `CollapsableNavSpec.js`
@yordis
Copy link

yordis commented Jun 27, 2015

I know doesn't matter at this state but I am not agree with this change. We use a lot of -able subfix for non english words and we are fine with it (e.g https://github.com/plataformatec/devise ). It's even a standards sometime create whatever word use this when you need to say that word is an action.

I don't thing this was a valuable time and that's what I really care about it. For me, this is weird now. 👎

@wmertens
Copy link
Author

In that case it should be CollapseAble. Javascript doesn't have cheap type
checking, let's not make things difficult by using non-existing words that
look like existing ones.

On Sat, Jun 27, 2015, 7:32 AM Yordis Prieto notifications@github.com
wrote:

I know doesn't matter at this state but I am not agree with this change.
We use a lot of -able subfix for non english words and we are fine with it
(e.g https://github.com/plataformatec/devise ). It's even a standards
sometime create whatever word use this when you need to say that word is an
action.

I don't thing this was a valuable time. For me, this is weird now. [image:
👎]


Reply to this email directly or view it on GitHub
#425 (comment)
.

Wout.
(typed on mobile, excuse terseness)

@AlexKVal
Copy link
Member

Just an additional side note.

CollapsibleMixin could be deprecated and removed at all in a couple of releases
( if @jquense would allow it to happen 😉 )

here is the proposal
#848

like here
https://github.com/react-bootstrap/react-bootstrap/pull/848/files#diff-c5a88cb9e192df4b2a4e2ed9a0a18884L3

@AlexKVal
Copy link
Member

I believe that React-Bootstrap's issues tracker is not an appropriate place for discussions about language nuances.

@yordis React-Bootstrap is an open source project, i.e. anybody can (and should 😉)
contribute their ideas to it.

If you have anything to bring in, move ahead and make a PR 😉
PRs are very welcome. ❇️

I'm locking this topic to prevent a 'flame'.
🍒

@react-bootstrap react-bootstrap locked and limited conversation to collaborators Jun 28, 2015
@jquense
Copy link
Member

jquense commented Jun 28, 2015

we all appreciate both sides to this issue and understand the cost of changing the names of things in terms of maintenance and time. in the future this mixin will most likely be deprecated and then removed so the whole thing is moot.

rather then let this issue get into the fraught area of discussing the meaning of words and the subjectivity of language we are just going to lock this and call the discussion finished, having heard both sides

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.