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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 馃幐 Proposal: Decoding JSON in RealityComposerStrategy #47

Merged
merged 5 commits into from Jun 21, 2021

Conversation

pgobriensap
Copy link
Contributor

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.

@pgobriensap pgobriensap requested a review from a team as a code owner June 18, 2021 03:00
BREAKING CHANGE: 馃Ж Renamed RealityComposerStrategy
@pgobriensap
Copy link
Contributor Author

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.

@MarcoEidinger MarcoEidinger requested review from billzhou0223 and MarcoEidinger and removed request for a team June 18, 2021 15:13
Copy link
Member

@MarcoEidinger MarcoEidinger left a 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

@pgobriensap
Copy link
Contributor Author

pgobriensap commented Jun 18, 2021

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?

Copy link
Member

@MarcoEidinger MarcoEidinger left a 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

Sources/FioriARKit/ARCards/Models/CardItemModel.swift Outdated Show resolved Hide resolved
Sources/FioriARKit/ARCards/Models/CardItemModel.swift Outdated Show resolved Hide resolved
Copy link
Member

@MarcoEidinger MarcoEidinger left a 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

Sources/FioriARKit/ARCards/Models/CardItemModel.swift Outdated Show resolved Hide resolved
@pgobriensap pgobriensap merged commit 6c14176 into SAP:main Jun 21, 2021
@pgobriensap pgobriensap deleted the newLoadingStrategies branch June 21, 2021 16:08
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.

None yet

2 participants