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

Feat/user metadata upload endpoint #13

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

eduairet
Copy link
Member

Hey @jsanchez034 I merged main into this branch, now it works, what do you think about merging it (most of the things happen in the create-edit-user and don't affect the rest of the dapp), and keep going with a fresh restart with all the refactoring we discussed in the PR #11

- Now you can fill the form in usertest.js UI and create a JSON in IPFS
Links field fixed
- Contract interactions checked
- Lens profile checked
Now you can create an user from the UI
Non connected behavior
Now the create/edit user for is finished
Set profile with metadata still not working
New name: create-edit-user
- Added the iron session api route to the metadata upload endpoint
Inputs removed from globals and added to Form.module.css
- Now create profile works with variables
- Missing ironOptions imported to endpoint
- Formik library and Yup implemented
- Default profile query when user creates profile
- Queries and mutations now work with variables
- Reducer form replaced by formik
- Metadata structure is now right but the mutation is not working
Now the user settings are get from lens if they exist
- Proxy contract added
- Edit user now works but it is sometimes underpriced
- Create profile query typo fixed
- Now you can access the create edit user window from the dashboard
- Input undefined error fixed
- Create edit user page now loads but needs a lot of debugging
- Connect contract matches JP's but needs to allow the edit user function
- Prettier and lint added to workflow
@eduairet eduairet requested a review from jsanchez034 January 25, 2023 06:30
@vercel
Copy link

vercel bot commented Jan 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
app-interplanetaryfonts ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 25, 2023 at 6:36AM (UTC)

Copy link
Contributor

@jsanchez034 jsanchez034 left a comment

Choose a reason for hiding this comment

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

This is awesome work Lalo! Thank you 😃

name: currentProfile.name,
...Object.fromEntries(
currentProfile.attributes.map((field) => {
return [field.key, field?.value || ""];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using field?.value here because field can be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, null or undefined so all the refs could start as string, I was getting a Some references started as undefined or something like that and this solved it, nevertheless I could find a better solution :)

Comment on lines +180 to +183
email: body?.email,
name: body?.name,
website: body?.website,
bio: body?.bio,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can body be null ( e.g. body? ) ? The yup validator library should take care of validating the form and not allowing this code to run no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this approach was after starting everything as string and I didn't change it, I need to check that, thanks for pointing it JP :)

<>
<h6>{lensHandle ? `Welcome ${lensHandle}` : "Welcome"}</h6>
{createEditUserInputs.map((field) => {
if (!lensHandle && field.id === "handle") {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this conditional are we saying to only render an input and error div combination of the the createEditUserInputs is the lens handle input? Should the conditional be field.id !== "handle" meaning all inputs besides the lens handle?

Copy link
Member Author

Choose a reason for hiding this comment

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

This part checks if the user has a lens profile, if is true loads all the data from that profile to the form, but this part needs to be taken from a global provider and not from this local approach, that's te main reason I want to leave this as it is temporarily, so I can still test with this frontend but start focusing on refactor and at some point come back here and chop the file, consume the global provider and make it more accessible to the user. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah got ya 👍 I tested out the pages and looks like if you have a lens profile the welcome message above is the only thing that is rendered...
image
So I think this conditional should be the following to render the form inputs...

Suggested change
if (!lensHandle && field.id === "handle") {
if ((lensHandle && field.id !== "handle") || !lensHandle) {

In other words render every field except the handle field if the user has a lens profile or render all fields if a user does not have a lens handle. Also I think once we are out of the testing phase and ready to launch the mvp we can remove the code in this file around handling creating a lens user. What do you think about adding todo comments in this file that link to a new task in our trello board to make sure we cleanup that code?

@@ -0,0 +1,9 @@
import classes from "../../styles/Form.module.css";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR but when refactoring/cleaning up the code, what do you think about colocating the css module files with the components, example...

- frontend
   - components
      - UI
         - Form.js
         - Form.module.css

This will make it easier to navigate to the css module files to make changes

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense, at the beginning it was simpler to manage the styles that way but as they increased everything got more complex haha

@@ -29,7 +30,9 @@ export default function ProfileData(props) {
{(props.user?.website ?? "").replace(/https?:\/\//, "")}
</a>
{props.connected && (
<button className={classes.edit}>Edit Profile</button>
<Link className={classes.edit} href="/create-edit-user">
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job using Link from next here 🙌

@@ -0,0 +1,367 @@
import React, { useState, useEffect } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

Another note for the refactor/cleanup task, we should break out component, functions and constants from this file into smaller files we import into this one to reduce the size of this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely agree :)

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