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

Progressive list rendering. #4098

Closed

Conversation

brockwhittaker
Copy link
Collaborator

Change longer lists to render progressively in pieces rather than all at once in order to not block up the UI.

@smarx
Copy link

smarx commented Mar 16, 2017

Automated message from Dropbox CLA bot

@brockwhittaker, it looks like you've already signed the Dropbox CLA. Thanks!

@brockwhittaker brockwhittaker force-pushed the progressive-list-rendering branch 3 times, most recently from 33629b2 to e9631b4 Compare March 16, 2017 22:50
$nearestScrollingContainer = $nearestScrollingContainer.parent();
}

var prototype = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this is a prototype in the strict JS sense, or is it just an object? We mostly just use closures for objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I named it as prototype since it will work like one from an outside perspective, but no. It could however be made into one -- which may be a good use case as this will be called many times perhaps and prototypes consume lots less memory (as only one copy exists in memory).

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't want to move toward having a prototype; my concern was simply that the name was a bit confusing. I prefer this style:

var self = {};

self.render = function () {....};
self.init = function () {...}
self.clear = function() {....};

@showell
Copy link
Contributor

showell commented Mar 17, 2017

I'm not doing a thorough review on this yet, but, broadly, this seems like a really nice encapsulation, so I like the general direction it's heading. I'd be curious to see how easily it could be extended to work for the buddy list, which is a little more dynamic in nature (but at the end of the day, just has single-item deletes/inserts to deal with, beyond what you've already done).

@brockwhittaker
Copy link
Collaborator Author

It would be possible on the list to proxy the array protoype and intercept incoming requests to display, but then again we may as well just use vue, in which case this closure will still exist but without the append to DOM code (just the append to DOM array code).

@brockwhittaker brockwhittaker force-pushed the progressive-list-rendering branch 2 times, most recently from 096953b to 142a195 Compare March 27, 2017 21:18
Brock Whittaker added 4 commits March 30, 2017 11:13
The height of the settings page content is not quite as tall as the
settings page could allow which makes for an empty white space at the
bottom of the settings content container.
This shows a preview of a node given a reference to it like:
<tag id=“id” class=“className”></tag>
This implements the list_render closure class that allows for
progressive, responsive rendering of long lists.
// container: jQuery object to append to.
// list: The list of items to progressively append.
// opts: An object of random preferences.
var func = function ($container, list, opts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change func to something like get_renderer? I also think we might want to separate two concerns here by having two functions:

memoization: make that be a function that simply keeps track of objects and has the 3/4 lines of code to make that happen
renderer: make that function be the object with the "guts" and don't have it track objects

@showell
Copy link
Contributor

showell commented Apr 12, 2017

We should definitely have a unit-testing strategy for the main piece of this, which is the lazy list rendering. If there are small pieces of this series that can be merged ahead of time, we should do try to work on getting those in first, and then spend some time making this code really testable. Node tests can handle some jQuery interactions ok, but it might make sense to have some functions that are easily stubbable, which might change how the code is organized a bit.

@timabbott
Copy link
Sponsor Member

This is really cool, but I'm not thrilled with how this has been developed, in that the last commit has a bunch of work that we're going to need to semi-redo due to merge conflicts, and that probably just shouldn't have been started until we had merged the framework and were happily using it in at least 1 place. Oh well.

To try to expedite progress on this project, I merged the first 3 commits (all but actually using the framework), after tweaking a bunch of comments and other style things in the actual rendering implementation. It's mostly harmless in that the code is unused, but I expect we'll need to do more work to make it work perfectly; but this should make it easy to work on the various pieces separately.

I think next we should proceed with 2 things in parallel:

  • Start merging few applications of this. @brockwhittaker can you extract out the changes for e.g. the "stream subscribers" UI and submit a PR just adding those? You only need to port the changes from the last commit (and given the changes, it might be easiest to just use the git show output for that last commit as a reference and redo things over trying to resolve merge conflicts). And then we can add additional widgets as we go.
  • I agree that ideally we'd write a good test suite for this library. @showell is probably best equipped to do that work; maybe makes sense to wait until we have 1 application working before starting on it. I imagine there'll be some refactoring and cleanup as part of that process.

Thanks for working on this @brockwhittaker! I'm going to close this PR -- we can use new PRs for further work on this.

@timabbott timabbott closed this Apr 14, 2017
@showell
Copy link
Contributor

showell commented Apr 14, 2017

@brockwhittaker @timabbott I'm happy to help on the unit tests. I agree that the merge conflicts were a bit unfortunate (my fault, although I felt the admin split took priority), and perhaps a more incremental approach would have been better, but this is also really exciting, so I look forward to more progress here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants