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

Component refactor and loading indicator stub #11

Merged
merged 18 commits into from
Jun 13, 2019

Conversation

vdwijngaert
Copy link
Contributor

See #6 and #10.

  • I refactored the code to one root component that contains the state and api logic.
  • Entities are being passed down as props to the child components (Schedule and LatestPosts)
  • I also had a go at improving the resilience of the fetching logic (enabling further enhancements)
  • Made it possible to save fetching state, and show a loading indicator based on that
  • Added a stub for displaying fetch-related errors.

@vdwijngaert
Copy link
Contributor Author

Note: it's my first time working with the WordPress implementation of React, hope I did okay.

Copy link
Contributor

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me 👍

There are a few things I think would help improve it, if there's time.

includes/page-templates.php Outdated Show resolved Hide resolved
includes/page-templates.php Outdated Show resolved Hide resolved
templates/assets/components/page.js Outdated Show resolved Hide resolved
templates/assets/components/page.js Outdated Show resolved Hide resolved
templates/assets/components/page.js Show resolved Hide resolved
templates/assets/components/page.js Outdated Show resolved Hide resolved
/**
* External dependencies
*/
import { sortBy } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the native sort() function may be able to replace this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the original code (by @avillegasn ?). I haven't looked at the internals, maybe something to look into in the future.

templates/assets/components/schedule.js Outdated Show resolved Hide resolved
templates/assets/components/schedule.js Show resolved Hide resolved
templates/assets/components/sessions.js Outdated Show resolved Hide resolved
@iandunn iandunn mentioned this pull request Jun 11, 2019
@vdwijngaert
Copy link
Contributor Author

Thanks for the insightful suggestions, @iandunn. Will try to implement these tonight and make sure everything merges.

@vdwijngaert
Copy link
Contributor Author

Aight, all suggestions are answered and/or implemented. Let's do this :)

Note: I also made some changes to the session logic. In my test environment all sessions were concluded in 2014, so I got a lot of errors when enabling the production environment. Right now, most properties should have proper fallbacks.

Now it displays "TRACK FINISHED" when there is no more session in the "up next" section. Now it shouldn't throw any errors, but we might want to change this to something more robust, or have some state for when all sessions are concluded.

@avillegasn avillegasn merged commit 022f8f3 into wceu:master Jun 13, 2019
@iandunn
Copy link
Contributor

iandunn commented Jun 13, 2019

LGTM :)

This was referenced Jun 14, 2019
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.

3 participants