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

feat: add new auth with magic.link #61

Merged
merged 22 commits into from
Apr 26, 2021
Merged

feat: add new auth with magic.link #61

merged 22 commits into from
Apr 26, 2021

Conversation

hugomrdias
Copy link
Contributor

No description provided.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 21, 2021 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: db30ed5
Status: ✅  Deploy successful!
Preview URL: https://6e6f8f61.nft-storage.pages.dev

View logs

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021 •
edited
Loading

Codecov Report

Merging #61 (5136fe9) into main (f6c394a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #61   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          244       244           
=========================================
  Hits           244       244

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a27c28...5136fe9. Read the comment docs.

alanshaw
alanshaw requested changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

NICE! Some minor issues, but I think we can get this merged ASAP!

README.md Outdated Show resolved Hide resolved
README.md Outdated
wrangler secret put AUTH0_DOMAIN --env production # Get from auth0 account
wrangler secret put AUTH0_CLIENT_ID --env production # Get from auth0 account
wrangler secret put AUTH0_CLIENT_SECRET --env production # Get from auth0 account
wrangler secret put _MAGIC_SECRET_KEY --env production # Get from auth0 account
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this secret prefixed with underscore?

site/src/constants.js Outdated Show resolved Hide resolved
site/src/models/users.js Outdated Show resolved Hide resolved
site/src/models/users.js Outdated Show resolved Hide resolved
website/components/loading.js Outdated Show resolved Hide resolved
website/lib/magic.js Outdated Show resolved Hide resolved
website/styles/global.css
display: block;
width: 100%;
overflow-x: auto;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These can almost all be tachyons classes, but can be fixed later.

website/components/button.js Outdated Show resolved Hide resolved
website/components/navbar.js Outdated
return (
<nav className={`dn dn-m db-ns bg-${bgColor}`}>
<nav className={` bg-${bgColor}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Those classes are there so the navbar disappears on mobile as it did not render well. We need a hamburger or something really. Could you open an issue to get it fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i will, i have tweak the navbar and its good enough for now in mobile

@alanshaw alanshaw mentioned this pull request Apr 26, 2021
Merged
alanshaw
alanshaw approved these changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Just some final comments, nothing particularly blocking so I've approved, my main concern is NEXT_PUBLIC_API and NEXT_PUBLIC_MAGIC which are undocumented and I can't see where they're set.

.github/workflows/client.yml
# GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
# NPM_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }}
# SRC_PACKAGE_DIR: client
# DEPLOY_DIR: client
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create an issue to re-instate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#66

site/src/constants.js Outdated Show resolved Hide resolved
client/src/lib.js
@@ -44,7 +44,7 @@ class NFTStorage {
*
* @param {{token: string, endpoint?:URL}} options
*/
constructor({ token, endpoint = new URL('https://nft.storage') }) {
constructor({ token, endpoint = new URL('https://api.nft.storage') }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ We need to redirect the old URLs in cloudflare for old clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already done for production

website/lib/api.js
@@ -0,0 +1,98 @@
import { getMagic } from './magic'

export const API = process.env.NEXT_PUBLIC_API
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this value documented/set?

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 docs in the readme

website/lib/magic.js
if (magic) {
return magic
}
magic = new Magic(process.env.NEXT_PUBLIC_MAGIC, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this value documented/set?

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 docs in the readme

@hugomrdias hugomrdias mentioned this pull request Apr 26, 2021
Closed
@hugomrdias hugomrdias merged commit 03c7c6f into main Apr 26, 2021
@hugomrdias hugomrdias deleted the feat/new-auth branch April 26, 2021 17:30
alanshaw pushed a commit that referenced this pull request Apr 27, 2021
feat: new KV to index NFTs ORDER BY user ID, created DESC (#65)
88faefc 
Adds a new KV that acts as an index of NFTs ordered by user ID then created. It allows us to list NFTs for a user in descending date order.

resolves #58

TODO:

* Needs #61 to merge for pagination in the UI (will send separate PR)
* ~~Will send separate PR for migration of existing NFTs~~ Added here
@alanshaw alanshaw mentioned this pull request Jul 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alanshaw alanshaw alanshaw 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
@hugomrdias @codecov-commenter @alanshaw