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

NFT Metadata form #2125

Merged
merged 45 commits into from
Jun 9, 2022
Merged

NFT Metadata form #2125

merged 45 commits into from
Jun 9, 2022

Conversation

FSM1
Copy link
Contributor

@FSM1 FSM1 commented May 11, 2022 •
edited
Loading

This is a very basic MVP of the NFT Metadata creation. The main objective here was to ensure that the basic requirements can be met. The UX can still do with a lot of improvement.

This addresses the initial part of #1931

@render
Copy link

render bot commented May 11, 2022

Your Render PR Server URL is https://chainsafe-components-stage-pr-2125.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c9tops7s437hl15oop1g.

@render
Copy link

render bot commented May 11, 2022

Your Render PR Server URL is https://files-ui-stage-pr-2125.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c9topt7s437hl15oop60.

@render
Copy link

render bot commented May 11, 2022

Your Render PR Server URL is https://storage-ui-stage-pr-2125.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c9topu7s437hl15oopng.

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label May 11, 2022
@lgtm-com
Copy link

lgtm-com bot commented May 11, 2022

This pull request introduces 3 alerts when merging c44d2cd into 0ae6827 - view on LGTM.com

new alerts:

  • 3 for Useless conditional

@FSM1 FSM1 requested review from Tbaut and tanmoyAtb May 11, 2022 12:20
Tbaut
Tbaut reviewed May 13, 2022
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Nice!! I left a couple comments in the code, I'll play with it now :)

packages/common-components/src/FileInput/FileInput.tsx Outdated Show resolved Hide resolved
packages/common-components/src/FileInput/FileInput.tsx Outdated Show resolved Hide resolved
packages/common-components/src/FileInput/FileInput.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Elements/FormikImageInput.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Elements/FormikImageInput.tsx Outdated Show resolved Hide resolved
packages/common-components/src/FileInput/FileInput.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Layouts/AppNav.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Pages/UploadNFT.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Pages/UploadNFT.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Pages/UploadNFT.tsx Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented May 17, 2022

This pull request introduces 3 alerts when merging c4f2d21 into 443c575 - view on LGTM.com

new alerts:

  • 3 for Syntax error

tanmoyAtb
tanmoyAtb requested changes May 17, 2022
View reviewed changes
Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Leaving a couple of comments here.
Looking into the UI.

packages/common-components/src/FileInput/FileInput.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Elements/FormikImageInput.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Components/Layouts/AppNav.tsx Outdated Show resolved Hide resolved
@FSM1 FSM1 requested a review from tanmoyAtb May 29, 2022 21:08
@Tbaut Tbaut self-requested a review May 31, 2022 14:08
Tbaut
Tbaut reviewed May 31, 2022
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Played with it locally and all tests passed but one. (the one that's marked as only right now)

files must be a array type, but the final value was: null. If "null" is intended as an empty value be sure to mark the schema as .nullable()

So I fixed this. The yarn issue was pretty weird, but I'll check it out more in detail if it keeps failing.

edit: I really think that's a red herring for a problem somewhere else. With a git --version it also fails to find git. But I don't think this is the origin of the problem, why would it appear now anyway.. see https://lifesaver.codes/answer/error-couldn-t-find-the-binary-git-2083

tanmoyAtb
tanmoyAtb approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Lets go with it 👍

Tbaut and others added 4 commits June 8, 2022 17:45
@Tbaut
nits and fix tests hopefully (#2172)
af922e7
@actions-user
lingui extract
43281f0
@Tbaut
Experimental PR - fixing git binary not found (#2178)
04bbdea 
* bump to v4

* nit to run github actions

* with debug and git

* with nit

* with nit

* fix yml

* +1

* gooo

* fix debug maybe

* with nit

* double debug

* without sudo

* yes to all

* to all
@Tbaut
Merge branch 'dev' into feat/nft-upload-form
f353ae5
Tbaut
Tbaut approved these changes Jun 9, 2022
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

uuuh yes

@Tbaut Tbaut merged commit bc4a7d7 into dev Jun 9, 2022
@Tbaut Tbaut deleted the feat/nft-upload-form branch June 9, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tbaut Tbaut Tbaut approved these changes

@tanmoyAtb tanmoyAtb tanmoyAtb approved these changes

Assignees
No one assigned
Labels
Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FSM1 @tanmoyAtb @Tbaut @actions-user