[RefreshIndicator] Port component #9614

leMaik · 2017-12-25T22:55:53Z

⚠️ This PR far from ready to merge, it's more like a draft. It's just here to get some early feedback.

This PR ports the RefreshIndicator from material-ui 0.x and brings a new RefreshableContainer component that provides a container that can be refreshed (hence the name, duh 😄) with pull-to-refresh.

Some of the things that still need to be done:

Add a separate example for RefreshableContainer
Allow RefreshableContainer to be used on document instead of its own div
Add a way to give RefreshableContainer a custom (top) offset for the refresh indicator
Add at least some tests
Bring test coverage to 💯
Verify that the animation and sizes match the specs
Add typings
Wait for feedback and add more checkboxes to this list

Related issues: #2833 and #6214 (the latter isn't really a duplicate, it asks for a HOC like the proposed RefreshableContainer)

oliviertassinari · 2017-12-25T23:12:42Z

@leMaik Thanks for working on it! I fear I won't have the time to deep dive into reviewing this PR anytime soon. I'm focusing on fixing bug with existing components. I hope people can jump in to help. At the end of the day I don't think that adding new components should be our priority. At least, it's not mine.

leMaik · 2017-12-26T00:03:07Z

At the end of the day I don't think that adding new components should be our priority. At least, it's not mine.

Well, I needed a refresh indicator for a project and some other people seem do be looking for one too, so I don't think it's a bad thing to add this feature. Focusing on fixing bugs is a good thing to do, though. 👍

oliviertassinari · 2017-12-26T09:18:24Z

I don't think it's a bad thing to add this feature

Wait, adding new components can harm, I have experienced it with v0.x. Either because it's increasing the code base, making refactorisation slower, introducting x new issues because the component wasn't polished, creating users and maintainers frustration. Worse, people jump to conclusion. One broken component can potentially mean the other are too. Supporting a new component is 1x for building it and 5x for maintaining it.

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

Antoine de Saint-Exupery

To sum up, if you are willing to maintaine the component in the long run, it's awesome and it worth going forward. Otherwise my strategy would rather be the TimePicker / DatePicker approach. i.e. Focusing on the core components for the next 6 months (stable v1).

leMaik · 2017-12-26T12:42:00Z

I agree that adding a new component brings some maintenance overhead, but this is really sad. 😞 Remember the table pagination? The expansion panel? Nobody refused it just because it was new.

That said, I see that the RefreshableContainer might be a pain to maintain. 😕 I'll remove it from this PR and put it into its own project. The RefreshIndicator isn't that hard, though and I'm willing to maintain it. 👍

oliviertassinari · 2017-12-26T12:52:52Z

Remember the table pagination? The expansion panel? Nobody refused it just because it was new.

@leMaik True and we can't support all the components features. I would rather support the expansion panel and the table pagination than the refresh indicator. From my experience, very few people use the indicator and ask for it. It was added in #1312. It has received very few feedback since then. Here the only implementation I'm aware of and nobody uses it: https://github.com/maoziliang/react-refresh-indicator.

leMaik · 2017-12-26T13:07:00Z

@oliviertassinari Following the last revision of your comment, I'll close this PR and implement the RefreshIndicator and the RefreshableContainer independently. Maybe it's a very uncommon use case indeed, but I currently need it, so I'll implement it (and not expect mui-org to maintain it).

oliviertassinari · 2017-12-26T13:23:40Z

@leMaik Wait, let's hear what the other maintainers want to do here :). This is just my point of view on the topic.

leMaik · 2017-12-26T13:27:43Z

"the less we do, the better we can do it".

I'm more like "the less we do, the more we have to do anyway but in seperate projects" 😄

Reopened, but RefreshableContainer will be moved.

oliviertassinari · 2017-12-26T13:31:24Z

"the less we do, the more we have to do anyway but in seperate projects"

Exactly 💯

oliviertassinari · 2017-12-26T16:34:35Z

cc @mui-org/core-contributors

rosskevin · 2017-12-26T16:51:21Z

I think a refreshable container is a great widget that others will use, but I do not know if it is a core component. Either way - it will be created and we want everyone to find/contribute to it in one place.

Either:
a) we accept it here; or
b) we have a new mui-org just for things like this, how about giving @leMaik a project under the org

The only problem with b is the overhead of creating a project for two component files (not to mention demos etc) - so I'm a bit torn as to the right solution. If the complexity is low (nothing like date/time picker or autosuggest), then I would lean towards accepting it here. If the complexity or use case variation is high, I think we should split it into a project.

One last option - create another mui-org project for incubation/unstable/advanced. That way the overhead problem is solved, and components can be used/shared/vetted prior to being accepted under core.

Thoughts?

mbrookes · 2017-12-27T02:33:35Z

I think a refreshable container is a great widget that others will use

I agree, and I'd suggest that perhaps it's absence on v0.x was part of the reason for the lack of interest in the refresh indicator component. By itself, that isn't particularly useful.

create another mui-org project for incubation/unstable/advanced

That strikes a nice balance for components which aren't initially accepted for mui-org/material-ui, with the possibility to "graduate" to MUI if proved valuable / stable / popular.

kgregory · 2017-12-27T02:49:10Z

I agree with @rosskevin and @mbrookes - the idea of a proving ground for new components would be great.

rosskevin · 2017-12-27T03:43:59Z

What is the simplest project skeleton + demo to get going? @oliviertassinari may want to stub it.

I propose the name is mui-org/denovo:

https://en.wikipedia.org/wiki/De_novo
published as @material-ui/denovo

The name is generic/abstract enough that it can be whatever it grows into.

kgregory · 2017-12-27T04:26:01Z

Names are hard. I was thinking this would be something like a collection of near-production prototypes fresh from the lab, or work table, work bench, etc. mui-org/bench features that are on the workbench, ready for use, not quite ready for the core package. Also similar to players waiting on the bench for their chance to prove themselves. Just offering this as an option, nothing against denovo

leMaik · 2017-12-27T10:59:49Z

What about creating one project/package per feature that is to be benchmarked? E.g. mui-org/refresh-indicator, so that users can also install that as a package?

Edit: Maybe, the component doesn't even need to be included in the core library. If the integration in the documentation is good enough, why not keep components that not everybody will need separated?

oliviertassinari · 2017-12-30T18:28:18Z

I'm proposing a plan going forward in #9673. Now, regarding the RefreshIndicator component, we have different options:

We put it in the lab.
@leMaik can host it with the other Wertarbyte components.
We can host a material-ui-refresh-indicator repository under the mui-org organization.

What do you guys prefer?

mbrookes · 2017-12-30T20:26:35Z

My vote is for ~~mui-org/lab~~ (npm: @material-ui/lab).

Edit: @oliviertassinari I just saw your monorepo comment in the "release v1" issue. I'm cool with that too. There's also git submodules...

Not precious about the name, just using it as a placeholder, although I'm not sure about "denovo". (Sorry @rosskevin!)

oliviertassinari · 2017-12-30T21:03:04Z

My vote is for mui-org/lab

@mbrookes So you would vote for having a different repository for the lab? What're the advantages? The disadvantages I can see:

We lose the commit history when promoting one component from lab to core.
We lose the contributors stats when promoting one component from lab to core.
We lose the issue history when promoting one component from lab to core.
It's creating coordination hell, you can't do a major core revamping in one go. You have to do it twice. First on the core. Then cross your finger for having it right with the lab.
We fragmentize the tools. Now, you have to look at two GitHub pages instead of one.
We fragmentize the activity. We will look smaller, less activity on the project.
We fragmentize the stars.

The advantages I can see:

Less noise, better scoping of notifications (can be solved differently)
More complex CI

mbrookes · 2017-12-30T22:38:15Z

Sorry, I edited my comment while you were writing yours. I'm with you on monorepo. It's a long time since I used submodules, but I think they may have the same disadvantages?

leMaik · 2017-12-31T11:48:19Z

@oliviertassinari I'd vote for 3, but @mui-org/refresh-indicator or 1. I'm not really a friend of the monorepo approach. What's the advantage of putting all non-core components into one project? Issues grow, there are tags needed to differentiate between components, …
I like the one-repo-one-component approach that TW currently uses. It increases the initial overhead to create a new component, though.

rosskevin · 2017-12-31T12:58:20Z

@leMaik I agree. Monorepo means issue noise/growth for things that may never be core, which rules it out in my opinion.

oliviertassinari · 2017-12-31T13:43:03Z

@leMaik Sounds good to me. Does #9673 (comment) address your concern?

areebiitr · 2018-02-20T06:43:53Z

src/RefreshIndicator/RefreshIndicator.js

+
+class RefreshIndicator extends React.Component {
+  getPaddingSize() {
+    const padding = this.props.size * 0.14;


Shouldn't we return padding directily.

return this.props.size * 0.14

The syntax used in this function and getPaperSize function should be same

Absolutely! 👍

areebiitr · 2018-02-20T06:45:32Z

src/RefreshIndicator/RefreshableContainer.js

+
+      if (touchNow > this.state.touchStart) {
+        // swiped down
+        e.preventDefault()


Why do we need event.preventDefault only in case of swipe down?

To prevent the browser's swipe-to-refresh function.

… even more if so

…roperties

leMaik · 2018-02-25T12:06:30Z

It's almost ready for the lab! 🎉

mbrookes · 2018-03-19T01:02:36Z

@leMaik What's the current status of this component? Is it nearly ready for review?

leMaik · 2018-03-21T13:06:50Z

@mbrookes I think it is, feel free to give me a review. 👍

I just want to improve the example, it looks a bit ugly.

skirunman · 2018-06-06T17:38:43Z

@kgregory EDIT: never mind...

~~Just FYI, but I think pull to refresh, so the RefreshIndicator component, is no longer in the new Material Design spec.~~ Still could be useful though to have in MUI lab.

kgregory · 2018-06-06T18:11:09Z

Just FYI, but I think pull to refresh, so the RefreshIndicator component, is no longer in the new Material Design spec.

What about: https://material.io/design/platform-guidance/android-swipe-to-refresh.html#usage

oliviertassinari · 2018-07-13T09:07:23Z

https://material.io/design/platform-guidance/android-swipe-to-refresh.html#usage

@kgregory Right, the refresh indicator is still in the specification. It's definitely some nice to have (I won't say it's a must-have).

This PR far from ready to merge, it's more like a draft. It's just here to get some early feedback.

@leMaik I'm closing the pull request for now. After more thoughts, I have nothing against accepting a refresh-indicator in the lab :). Feel free to complete the implementation! I haven't reviewed any of lab components yet. I think that it's healthy. I shouldn't be a bottleneck. It's also giving space and freedom for people to jump in.

jacobweber · 2018-08-15T20:05:04Z

Is someone developing this somewhere else, where I could take a look? I'm having trouble finding a decent pull-to-refresh implementation that doesn't result in jerky scrolling on iOS (or lack of momentum scrolling).

leMaik added the component: RefreshIndicator label Dec 25, 2017

leMaik self-assigned this Dec 25, 2017

oliviertassinari added the new feature New feature or request label Dec 25, 2017

leMaik changed the title ~~[RefreshIndicator] Port component + Add RefreshableContainer~~ [RefreshIndicator] Port component Dec 26, 2017

leMaik closed this Dec 26, 2017

leMaik reopened this Dec 26, 2017

oliviertassinari mentioned this pull request Dec 30, 2017

Let's ship v1! #9673

Closed

oliviertassinari added the package: lab Specific to @mui/lab label Feb 3, 2018

leMaik force-pushed the refresh-indicator branch from 2562c8a to b040d5c Compare February 5, 2018 21:39

oliviertassinari added this to the post v1 milestone Feb 11, 2018

areebiitr suggested changes Feb 20, 2018

View reviewed changes

leMaik added 10 commits February 25, 2018 11:22

[RefreshIndicator] Start porting the component from material-ui 0.x

ce06e64

[RefreshIndicator] Tweak size

c170573

[RefreshableContainer] Add small proof-of-concept implementation

172221d

[RefreshIndicator] Cleanup unneeded code from port

243c270

[RefreshableContainer] Check if scroll is at top, improve animation

645aca3

[RefreshIndicator] Allow percentage to be over 100 and turn the arrow…

339f817

… even more if so

[RefreshableContainer] Don't limit indicator percentage to 100

f319a6e

[RefreshableContainer] Add attachToWindow property and document the p…

c722bc7

…roperties

[RefreshIndicator] Minor syntax tweak

52909fb

[RefreshIndicator] Move the components to the lab

25a9d9a

leMaik force-pushed the refresh-indicator branch from b040d5c to 25a9d9a Compare February 25, 2018 11:09

leMaik added 2 commits February 25, 2018 12:45

[RefreshIndicator] Update and simplify the colors

72ae3b5

[RefreshIndicator] Lint the code, drop old documentation file

891efd6

[RefreshIndicator] Lint code, add margin to demo refresh indicators

469909f

oliviertassinari changed the base branch from v1-beta to master May 12, 2018 18:19

oliviertassinari removed this from the post v1.0.0 milestone May 20, 2018

oliviertassinari closed this Jul 13, 2018

oliviertassinari added out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) and removed package: lab Specific to @mui/lab labels Jan 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RefreshIndicator] Port component #9614

[RefreshIndicator] Port component #9614

leMaik commented Dec 25, 2017 •

edited

Loading

oliviertassinari commented Dec 25, 2017 •

edited

Loading

leMaik commented Dec 26, 2017 •

edited

Loading

oliviertassinari commented Dec 26, 2017 •

edited

Loading

leMaik commented Dec 26, 2017

oliviertassinari commented Dec 26, 2017 •

edited

Loading

leMaik commented Dec 26, 2017

oliviertassinari commented Dec 26, 2017 •

edited

Loading

leMaik commented Dec 26, 2017 •

edited

Loading

oliviertassinari commented Dec 26, 2017

oliviertassinari commented Dec 26, 2017

rosskevin commented Dec 26, 2017

mbrookes commented Dec 27, 2017 •

edited

Loading

kgregory commented Dec 27, 2017

rosskevin commented Dec 27, 2017 •

edited

Loading

kgregory commented Dec 27, 2017

leMaik commented Dec 27, 2017 •

edited

Loading

oliviertassinari commented Dec 30, 2017

mbrookes commented Dec 30, 2017 •

edited

Loading

oliviertassinari commented Dec 30, 2017 •

edited

Loading

mbrookes commented Dec 30, 2017

leMaik commented Dec 31, 2017 •

edited

Loading

rosskevin commented Dec 31, 2017

oliviertassinari commented Dec 31, 2017

areebiitr Feb 20, 2018 •

edited

Loading

leMaik Feb 25, 2018

areebiitr Feb 20, 2018

leMaik Feb 25, 2018

leMaik commented Feb 25, 2018

mbrookes commented Mar 19, 2018

leMaik commented Mar 21, 2018 •

edited

Loading

skirunman commented Jun 6, 2018 •

edited

Loading

kgregory commented Jun 6, 2018

oliviertassinari commented Jul 13, 2018 •

edited

Loading

jacobweber commented Aug 15, 2018

[RefreshIndicator] Port component #9614

[RefreshIndicator] Port component #9614

Conversation

leMaik commented Dec 25, 2017 • edited Loading

oliviertassinari commented Dec 25, 2017 • edited Loading

leMaik commented Dec 26, 2017 • edited Loading

oliviertassinari commented Dec 26, 2017 • edited Loading

leMaik commented Dec 26, 2017

oliviertassinari commented Dec 26, 2017 • edited Loading

leMaik commented Dec 26, 2017

oliviertassinari commented Dec 26, 2017 • edited Loading

leMaik commented Dec 26, 2017 • edited Loading

oliviertassinari commented Dec 26, 2017

oliviertassinari commented Dec 26, 2017

rosskevin commented Dec 26, 2017

mbrookes commented Dec 27, 2017 • edited Loading

kgregory commented Dec 27, 2017

rosskevin commented Dec 27, 2017 • edited Loading

kgregory commented Dec 27, 2017

leMaik commented Dec 27, 2017 • edited Loading

oliviertassinari commented Dec 30, 2017

mbrookes commented Dec 30, 2017 • edited Loading

oliviertassinari commented Dec 30, 2017 • edited Loading

mbrookes commented Dec 30, 2017

leMaik commented Dec 31, 2017 • edited Loading

rosskevin commented Dec 31, 2017

oliviertassinari commented Dec 31, 2017

areebiitr Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

leMaik Feb 25, 2018

Choose a reason for hiding this comment

areebiitr Feb 20, 2018

Choose a reason for hiding this comment

leMaik Feb 25, 2018

Choose a reason for hiding this comment

leMaik commented Feb 25, 2018

mbrookes commented Mar 19, 2018

leMaik commented Mar 21, 2018 • edited Loading

skirunman commented Jun 6, 2018 • edited Loading

kgregory commented Jun 6, 2018

oliviertassinari commented Jul 13, 2018 • edited Loading

jacobweber commented Aug 15, 2018

leMaik commented Dec 25, 2017 •

edited

Loading

oliviertassinari commented Dec 25, 2017 •

edited

Loading

leMaik commented Dec 26, 2017 •

edited

Loading

oliviertassinari commented Dec 26, 2017 •

edited

Loading

oliviertassinari commented Dec 26, 2017 •

edited

Loading

oliviertassinari commented Dec 26, 2017 •

edited

Loading

leMaik commented Dec 26, 2017 •

edited

Loading

mbrookes commented Dec 27, 2017 •

edited

Loading

rosskevin commented Dec 27, 2017 •

edited

Loading

leMaik commented Dec 27, 2017 •

edited

Loading

mbrookes commented Dec 30, 2017 •

edited

Loading

oliviertassinari commented Dec 30, 2017 •

edited

Loading

leMaik commented Dec 31, 2017 •

edited

Loading

areebiitr Feb 20, 2018 •

edited

Loading

leMaik commented Mar 21, 2018 •

edited

Loading

skirunman commented Jun 6, 2018 •

edited

Loading

oliviertassinari commented Jul 13, 2018 •

edited

Loading