This repository was archived by the owner on Feb 2, 2024. It is now read-only.
Simplify metadata logic #559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Pull Request Test Coverage Report for Build 5591965705
💛 - Coveralls |
|
@shoom3301 @elena-zh I will merge this optimistically, I hope it's not too bad. |
shoom3301
reviewed
Jul 19, 2023
| _toFullAppData(appDataForm) | ||
| .then((fullAppData) => { | ||
| // Update the fullAppData | ||
| setFullAppData(fullAppData) |
There was a problem hiding this comment.
Why do we do _toFullAppData(appDataForm).then(setFullAppData) twice?
This was referenced Jul 19, 2023
alfetopito
reviewed
Jul 19, 2023
Comment on lines
+258
to
+259
| <RowWithCopyButton className="appData-hash" textToCopy={fullAppData} contentsToDisplay={fullAppData} /> | ||
| <p className="disclaimer">Note: Don’t forget to upload this file to IPFS!</p> |
There was a problem hiding this comment.
This is not a requirement anymore when it's submitted to backend, right?
| </a>{' '} | ||
| </p> | ||
| <p> | ||
| What this means is that you can derived the IPFS CID from on-chain CoW Orders, and download the JSON |
| </p> | ||
| <p> | ||
| What this means is that you can derived the IPFS CID from on-chain CoW Orders, and download the JSON | ||
| from IPFS network to see the meta-information of that order |
There was a problem hiding this comment.
Also, the backend now hosts and serves this info.
Should that be mentioned here?
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related to cowprotocol/app-data#32
Since i was working on the app-data, and did some simplifications on the library, i took this chance to remove some weird logic from the DOC generation.
This code is still messy, but at least it should work more automatically out of the box when we add new meta-data.
This PRs adds the UTM codes, order class, and all the missing types:
TLDR
Turns this
Into this:
DEMO
Screen.Recording.2023-07-18.at.20.34.39.mov
Adds Prettied + Actual FullAppData + AppData hex + CID
Just for completeness adds all of that, and give some context on what is each.
It tried to be interactive, so you experiment to change some fields
Dinamic
Also makes it dynamic (you don't need to send the form, it will update automatically)
This way, you see right away how changing some parameters affects the result
Improved validation
I suppressed some annoying "required" errors you get because of the metadata and how this library work.
Removing the annoying "enable field"
It was super bad UX to enable fields in order to use them.
Also, because this was done programmatically, it was making it hard to add new metadata. You will need to handle the logic of enabling those too.
Test
Compare this PR and PRO: