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

User page #323

Merged
merged 2 commits into from Feb 6, 2018
Merged

User page #323

merged 2 commits into from Feb 6, 2018

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Jan 31, 2018

User page. Branched off of #313, so shouldn't be merged before it.
@akarve @asah pls review

Questions:

  • now the api returns ok response (with empty package list) even if the user doesn't exist -- is it ok or should we check it somehow and show some "user doesn't exist" message?
  • should we show something else on the page? task description in asana suggests that there should be "browse all packages" link and gallery, is that right?

@akarve
Copy link
Member

akarve commented Feb 2, 2018

@nl0

  • for the time being go ahead and handle the error (even if it doesn't occur) e.g. as we do in containers/App/sagas.js in doGetPackage; and make sure the component renders the error condition using the existing classes (e.g. this link for a package that doesn't exist)
  • just list the packages for now. page title should be Packages owned by username

@akarve
Copy link
Member

akarve commented Feb 2, 2018

@nl0 also, if the list is empty, do something like this. I believe PackageList provides a default string for that (it should, anyway).

@nl0
Copy link
Member Author

nl0 commented Feb 5, 2018

@akarve

for the time being go ahead and handle the error (even if it doesn't occur) e.g. as we do in containers/App/sagas.js in doGetPackage; and make sure the component renders the error condition using the existing classes (e.g. this link for a package that doesn't exist)

the error is handled properly on the front-end, but the thing is that the api doesn't return error for the non-existent user, it just returns an empty package list, which doesn't seem correct.

also, if the list is empty, do something like this. I believe PackageList provides a default string for that (it should, anyway).

PackageList already does this (it says "Nothin here yet").

@nl0
Copy link
Member Author

nl0 commented Feb 5, 2018

page title should be Packages owned by username

done

@asah asah requested a review from akarve February 5, 2018 17:45
@nl0
Copy link
Member Author

nl0 commented Feb 5, 2018

i'll rebase this and double-check if everything's ok

@akarve
Copy link
Member

akarve commented Feb 5, 2018

@nl0 branch is approved but you'll need to resolve the conflicts before i can merge

@akarve
Copy link
Member

akarve commented Feb 5, 2018

BTW - API is behaving as expected; please still test the failure case (status !== 200) and error chain, but for now it's OK if API is 200 even if user doesn't exist.

`owner` prop is required for `PackageHandle`, but we don't always have
the owner set on objects in the list, so we need a way to specify their
common owner.

Also, default `emptyMessage` has been moved to `defaultProps` for
consistency.
@nl0
Copy link
Member Author

nl0 commented Feb 6, 2018

@akarve ready. As with #302, this PR is incompatible with #326, so if one is merged, the other one should be adjusted before merging.

@akarve akarve merged commit 3718c73 into quiltdata:master Feb 6, 2018
@nl0 nl0 deleted the user-page branch February 6, 2018 19:26
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.

None yet

2 participants