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

Infinite scroll #4

Merged
merged 8 commits into from
Feb 12, 2019
Merged

Infinite scroll #4

merged 8 commits into from
Feb 12, 2019

Conversation

s-kennedy
Copy link
Contributor

@s-kennedy s-kennedy commented Feb 8, 2019

The only issue I'm having is that I'm getting this warning on the P2PU website on the courses page whenever it fetches another set of results:

Warning: getDefaultProps is only used on classic React.createClass definitions. Use a static property named `defaultProps` instead.

... but I don't the same warning with the demo in the package. It's coming from the common bundle. So I don't think it's from this package, but i'm not 100% sure.

Anyway @dirkcuys do you want to check this over before I merge it? I ditched rollup and went with webpack so we could have a dev server and a demo for development purposes.

@s-kennedy
Copy link
Contributor Author

s-kennedy commented Feb 8, 2019

I think the warning is coming up because it's mixing the production version of create-react-class and the development version of react...
More info: facebook/react#9999 (comment)

@@ -8,8 +8,10 @@ class BrowseCourses extends React.Component {
}

componentDidUpdate(prevProps) {
if (prevProps != this.props) {
$('[data-toggle="tooltip"]').tooltip();
if (process.env.NODE_ENV === "production") {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to check for production / development environment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh yeah, good point, that was just to get the demo working because i didn't have bootstrap and jquery loaded, but i added them and removed this check.

@dirkcuys
Copy link
Member

@s-kennedy thanks, I'll have a look.

I remember before there was some difficulty in using webpack rather than rollup, Was it just an issue of configuring webpack, or did webpack add something new that makes it easier to build libraries?

@dirkcuys
Copy link
Member

The infinite scroll works nicely, I had to change the DEFAULT_ORIGIN variable in constants.js to pull from learningcircles.p2pu.org to see what it looks like with many courses. How are we configuring that on www.p2pu.org?

@s-kennedy
Copy link
Contributor Author

@s-kennedy thanks, I'll have a look.

I remember before there was some difficulty in using webpack rather than rollup, Was it just an issue of configuring webpack, or did webpack add something new that makes it easier to build libraries?

I switched to webpack so I could set up the dev server and a demo page, that's not available in rollup.

@s-kennedy
Copy link
Contributor Author

The infinite scroll works nicely, I had to change the DEFAULT_ORIGIN variable in constants.js to pull from learningcircles.p2pu.org to see what it looks like with many courses. How are we configuring that on www.p2pu.org?

it' just hard coded in the constants, I updated that, thanks.

@s-kennedy s-kennedy merged commit 312e33a into master Feb 12, 2019
@dirkcuys dirkcuys deleted the infinite_scroll branch February 13, 2019 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants