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

[Ionic v4] ion-refresher has no property enabled #14879

Closed
cvaliere opened this issue Jul 26, 2018 · 6 comments
Closed

[Ionic v4] ion-refresher has no property enabled #14879

cvaliere opened this issue Jul 26, 2018 · 6 comments
Assignees
Labels
package: core @ionic/core package

Comments

@cvaliere
Copy link

cvaliere commented Jul 26, 2018

Ionic Info
@ionic/angular: "4.0.0-beta.0"

Describe the Bug
ion-refresher has no property enabled as it had in v3

Expected solution
Put it back or provide guidance about how to migrate in the breaking changes list

@ionitron-bot ionitron-bot bot added the triage label Jul 26, 2018
@brandyscarney
Copy link
Member

Thanks for the issue! Right now disabled is the property to use, but I'd like to bring this up for discussion.

@ionic-team/framework - I know we changed this to make disabled the consistent attribute across the components, but I am kind of thinking in the two cases where disabled defaults to true it should be changed to enabled defaulting to false. Just so by default the attribute is not there & adding it makes it true.

screen shot 2018-07-26 at 11 19 18 am

@brandyscarney brandyscarney added the package: core @ionic/core package label Jul 26, 2018
@ionitron-bot ionitron-bot bot removed the triage label Jul 26, 2018
@mhartington
Copy link
Contributor

mmm, I think it should stay the same.

Having most of the components use disabled as the prop name and 2-3 use enabled makes people have to second guess when to use the right one. By keeping things as disabled, this is not an issue.

If we made the switch, i'd rather it be all or nothing.

@cvaliere
Copy link
Author

my 2 cents: why refresher is disabled by default? it seems to me that in v3 it was enabled by default, wasn't it?

@brandyscarney
Copy link
Member

brandyscarney commented Jul 26, 2018

@cvaliere Hmm you are correct. 🤔

https://github.com/ionic-team/ionic/blob/v3/src/components/refresher/refresher.ts#L103

I think we need to switch the default back. It seems counterintuitive to set disabled="false" by default.

@manucorporat Any reason you can think of for why we shouldn't do this?

@brandyscarney brandyscarney self-assigned this Jul 26, 2018
@cvaliere
Copy link
Author

thank you, that was really fast!

you can look at the others issues I created today, it's all the same kind of stuff 😁

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 1, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package
Projects
None yet
Development

No branches or pull requests

3 participants