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

Add new component: <TheFold /> #221

Merged
merged 2 commits into from
May 26, 2016
Merged

Add new component: <TheFold /> #221

merged 2 commits into from
May 26, 2016

Conversation

gigabo
Copy link
Contributor

@gigabo gigabo commented May 26, 2016

This replaces the getAboveTheFoldCount() page method, which is awful:

  • It's separate from the elements themselves, so even remembering to update it
    can be a challenge.
  • If the number of root elements above the fold varies, then duplicative logic
    may be required to determine the count.
  • Middleware that inserts elements must remember to increment it.
  • It has non-obvious behavior when root containers are used above the fold (RootContainers count twice for getAboveTheFoldCount #144)

This closes #161 where @bartkusa proposed this much nicer interface.

This is a breaking change. The default was previously to wake up the client
controller after the first element. With this change the client controller
isn't bootstrapped until either <TheFold /> is seen, or we're out of
elements. So, if unspecified, the entire page is assumed to be above the
fold.

This replaces the `getAboveTheFoldCount()` page method, which is awful:

- It's separate from the elements themselves, so even remembering to update it
  can be a challenge.
- If the number of root elements above the fold varies, then duplicative logic
  may be required to determine the count.
- Middleware that inserts elements must remember to _increment_ it.
- It has non-obvious behavior when root containers are used above the fold (redfin#144)

This closes redfin#161 where @bartkusa proposed this _much_ nicer interface.

This is a breaking change.  The _default_ was previously to wake up the client
controller after the _first_ element.  With this change the client controller
isn't bootstrapped until either `<TheFold />` is seen, or we're out of
elements.  So, if unspecified, the entire page is assumed to be above the
fold.
@gigabo gigabo added enhancement New functionality. breaking change labels May 26, 2016
@gigabo gigabo added this to the v0.3.0 milestone May 26, 2016
@@ -8,8 +11,8 @@ export default class DelayDataPage {
return 'application/json';
}
getResponseData() {
const { ms, val } = this.getRequest().getQuery();
const { ms, val, big } = this.getRequest().getQuery();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a big=<size> param to the test data endpoint that just tells it to return a JSON object with size keys. It's easiest to see the fold when there's a bunch of JS to for the browser to slurp up right after.

@roblg
Copy link
Member

roblg commented May 26, 2016

Looks reasonable to me. This is way nicer than getAboveTheFoldCount(). Three cheers for @bartkusa!

}
}

TheFold.defaultProps = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

const isTheFold = Symbol();
TheFold.defaultProps = { [isTheFold]: true };
export function isTheFold(e) => e && e.props && e[isTheFold];

is, I think, privater

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I actually thought about that. AFAIK the reason React does that is to avoid injection attacks via JSON. I don't see that as a concern here, so I opted to just keep it simple.

Maybe we _should_, but we didn't previously.  This preserves existing behavior.
@doug-wade
Copy link
Collaborator

👍

@gigabo
Copy link
Contributor Author

gigabo commented May 26, 2016

Note that this is merging into next. Once I merge this I'll rebase next onto master and look at publishing a prerelease of 0.3.0.

@gigabo gigabo merged commit de4d0a2 into redfin:next May 26, 2016
@gigabo gigabo deleted the the-fold branch May 26, 2016 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants