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.

Sign up for GitHub

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

Jump to bottom

Server-side rendering for feed and proper error handling #1199

Merged
merged 15 commits into from
Dec 15, 2017
Merged

Server-side rendering for feed and proper error handling #1199

merged 15 commits into from
Dec 15, 2017

Conversation

Sekhmet
Copy link
Contributor

@Sekhmet Sekhmet commented Dec 14, 2017 •
edited
Loading

Fixes #815
Fixes #1100
Fixes #1147

Changes:

  • Handle login exception
  • Change how TopicSelector state is handled (makes it less hacky and work flawlessly on browser and SSR)
  • Fetch and render feed on server.
  • Update 401 and 404 pages.
  • Return proper HTTP status code for 401 and 404 pages.
  • Handle 404 for feed.
  • Handle 404 for posts.
  • Handle 404 for user profiles.

User not found:
https://busy-master-pr-1199.herokuapp.com/@notsekhmet
Post not found:
https://busy-master-pr-1199.herokuapp.com/dlive/@dlive/announcing-dlive-nope
Feed not found:
https://busy-master-pr-1199.herokuapp.com/weird/bitcoin

@bonustrack bonustrack temporarily deployed to busy-master-pr-1199 December 14, 2017 23:35 Inactive
jm90m
jm90m approved these changes Dec 15, 2017
View reviewed changes
Copy link
Contributor

@jm90m jm90m left a comment

Choose a reason for hiding this comment

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

hey this LGTM, but one side note, when we go to invalid pages like this https://busy-master-pr-1199.herokuapp.com/test1/steemiteducation, its just a blank screen, think we can add like a 404 error page / generic page? I guess it can be done in another PR though :)

@Sekhmet
Copy link
Contributor Author

Sekhmet commented Dec 15, 2017 •
edited
Loading

Actually there are two issues (#815, #1100) that I have assigned myself to that refer to that problem.
I think I can do this in this PR.

@Sekhmet Sekhmet changed the title Server-side rendering for feed Server-side rendering for feed and proper error handling Dec 15, 2017
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1199 December 15, 2017 13:33 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1199 December 15, 2017 17:21 Inactive
@Sekhmet Sekhmet removed the PR: WIP label Dec 15, 2017
bonustrack
bonustrack approved these changes Dec 15, 2017
View reviewed changes
Copy link
Contributor

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

LGTM

@jm90m
Copy link
Contributor

jm90m commented Dec 15, 2017

same lgtm as well

@Sekhmet Sekhmet merged commit a318b8d into busyorg:master Dec 15, 2017
@Sekhmet Sekhmet deleted the feed-ssr branch December 15, 2017 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonustrack bonustrack bonustrack approved these changes

@jm90m jm90m jm90m approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sekhmet @jm90m @bonustrack