-
Notifications
You must be signed in to change notification settings - Fork 8
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: 🎸 Proposal: Decoding JSON in RealityComposerStrategy #47
Conversation
BREAKING CHANGE: 🧨 Edited Convenience Init
BREAKING CHANGE: 🧨 Renamed RealityComposerStrategy
After thinking about it I could have the CardItem generic in the Strategy Conform to Decodable. This would cause the strategy to need an explicit declaration of the CardItem Type. However perhaps a DefaultCardItem is still appealing in case the developers only want the default content. So they don't have to worry about creating a custom Decoding for their concrete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not commit Tests.json
and therefore the example app cannot be build out-of-the-box
Yeah, good catch. It was just a reference to the file on my disk and not added into the project directory. It has been pushed. I also decided to use base64 Encoding as the json value for the image since Bill mentioned it. Should there also be an alternative where the value could be a string to a file path of an image? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also decided to use base64 Encoding for the image. Should there also be an alternative where the image could be a string to a file path of an image?
Let's leave it for base64 only
So they don't have to worry about creating a custom Decoding for their concrete.
Yeah, I think that's the main benefit here. In the end they can decode their custom type and pass it as long as it conforms to CardItemModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please delete unused code and do renaming
BREAKING CHANGE: 🧨 Simplified RealityComposerStrategy Init/Renamed Errors
Proposal: Init to accept json Data
To do this I needed a Concrete Implementation of CardItemModel to conform to Decodable.
As a result when using JSON to load data the developer is constrained to
DefaultCardItem
Open to better naming convention
If proposal is Accepted I can create an Encodable Init when my bandwidth increases.
As for breaking changes, I experienced some of these errors and they weren't clear without Error appended. The init for object anchors was just for convenience and didn't add anything. Documentation can clarify this.