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

[RefreshIndicator] Add new component. #1312

Merged
merged 3 commits into from
Aug 4, 2015
Merged

[RefreshIndicator] Add new component. #1312

merged 3 commits into from
Aug 4, 2015

Conversation

maoziliang
Copy link
Contributor

I just merge my component into material-ui. But the implementation of my original and the current is different, by the animation implementation. css3 version against setTimeout version.
Perhaps the component's name is not suitabe. And the code need to be reviewed.
More detail code and examples at here: RefreshIndicator

@@ -20,6 +20,7 @@ class Components extends React.Component {
{ route: 'menus', text: 'Menus'},
{ route: 'paper', text: 'Paper'},
{ route: 'progress', text: 'Progress'},
{ route: 'refresh-indicator', text: 'RefreshIndicator'},
Copy link
Member

Choose a reason for hiding this comment

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

Please add a space to the text 'Refresh Indicator`

name: 'percentage',
type: 'number',
header: 'default: 0',
desc: 'The confirmation progress to fetch data. Max value is 100'
Copy link
Member

Choose a reason for hiding this comment

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

According to the design specs, the refresh arrow doesn't seem to indicate percent complete progress - rather, it's just part of a swipe animation - https://www.google.com/design/spec/patterns/swipe-to-refresh.html#swipe-to-refresh-swipe-to-refresh

I'm not sure if this actually changes the implementation of this component since we can still use this prop to implement the swipe animation. However, it does change how we explain this prop and what we name it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for your patience.

In fact, It can be set status to be loading at any percentage in the container component. The defination is my own opinion, and the arrow will be highlighted when percentage equals 100.

Android's SwipeRefreshLayout component has setDistanceToTriggerSync and setRefreshing methods. They almost have the same meaning with my implementation.

The difference between my implementation and specs is the highlight behavior. I add it because I think it can make much sense to the user.

@hai-cea
Copy link
Member

hai-cea commented Aug 3, 2015

@maoziliang Thank you for this PR! Please see my comments above.

@maoziliang
Copy link
Contributor Author

I'll merge the commits into one if every thing is ok.

AutoPrefix.set(wrapper.style, "transitionDuration", "0ms");

setTimeout(() => {
AutoPrefix.set(wrapper.style, "transform", "rotate(1800deg)");
Copy link
Member

Choose a reason for hiding this comment

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

May be we should assert that the component is still mounted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliviertassinari Thanks! I'll fix it. But the isMounted() method maybe deprecated in the future facebook/react#3220. However, I think it's ok to use it at this version.

hai-cea pushed a commit that referenced this pull request Aug 4, 2015
@hai-cea hai-cea merged commit ea7eb84 into mui:master Aug 4, 2015
@hai-cea
Copy link
Member

hai-cea commented Aug 4, 2015

Thanks @maoziliang !

@maoziliang maoziliang deleted the feature-refresh-indicator branch August 5, 2015 03:42
@zannager zannager added the docs Improvements or additions to the documentation label Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants