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

added display-type to generated serialized json for attributes #128

Merged

Conversation

bjartek
Copy link
Contributor

@bjartek bjartek commented Oct 11, 2024

Description

The generated json does not include display_type

Sorry for whitespace mod here. lmk if you want me to redo.

@bjartek bjartek requested a review from a team as a code owner October 11, 2024 18:14
@sisyphusSmiling sisyphusSmiling self-requested a review October 11, 2024 22:44
.concat(", \"value\": ").concat(value!)
.concat("}")
.concat("\"trait_type\": ").concat(Serialize.tryToJSONString(trait.name)!)
.concat(", \"display_type\": ").concat(Serialize.tryToJSONString(trait.display_type)!)
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling Oct 17, 2024

Choose a reason for hiding this comment

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

Since this value is optional in both Cadence and in OpenSea's standard, I'm wondering if we should conditionally include this here. Otherwise the serialized field may contain nil values for a field that is listed as optionally included.

Suggested change
.concat(", \"display_type\": ").concat(Serialize.tryToJSONString(trait.display_type)!)
.concat(", \"display_type\": ").concat(Serialize.tryToJSONString(trait.displayType)!)

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and good call including the display_type value. I do wonder if we should only include a serialized trait.displayType if it's not nil.

@bjartek
Copy link
Contributor Author

bjartek commented Oct 18, 2024

That is probably wise yes

@sisyphusSmiling
Copy link
Contributor

@bjartek in the interest of moving the PR along and make the suggested changes, I'll update to target a feature branch and pick up the PR

@sisyphusSmiling sisyphusSmiling changed the base branch from main to gio/update_display_type October 22, 2024 22:57
@sisyphusSmiling sisyphusSmiling merged commit a5630a7 into onflow:gio/update_display_type Oct 22, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants