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

Add user info to assetViewModel (Breaking Change) #201

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

nancywu1
Copy link
Contributor

@nancywu1 nancywu1 commented Oct 24, 2023

Model can now access the userInfo which can contain information set by the user usually through the plugins (SwiftUIPendingTransactionPlugin, SwiftUIBeaconContextPlugin) which will come later in OSS

Change Type (required)

Indicate the type of change your pull request is:

  • patch
  • minor
  • major

Version

Published prerelease version: 0.4.0-next.12

Changelog

Release Notes

Android: add note to input ref asset (#192)

Add note field to input reference asset

Export InfoAsset as XLR View (#139)

Export Info Asset as XLR View

sync up to latest (#131)

Sync latest code

Expose utils for expression LSP (#117)

  • Expose parseExpression from the expression parser
  • Adds a strict flag to parseExpression that will (when disabled) attach any parsing errors to the top-level node instead of throwing

Deps/bump dependencies (#111)

Dependency upgrades

Sync Latest Player Code (#109)

Syncs to latest version and bumps tapable-ts to latest version.


🚀 Enhancement

🐛 Bug Fix

📝 Documentation

Authors: 15

@nancywu1 nancywu1 requested a review from hborawski as a code owner October 24, 2023 20:24
Copy link
Contributor

@hborawski hborawski left a comment

Choose a reason for hiding this comment

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

*/
public required init(_ data: T) {
public required init(_ data: T, userInfo: [CodingUserInfoKey: Any]? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be optional? or default to [:]?

@@ -75,7 +80,7 @@ open class ControlledAsset<DataType: AssetData, ModelType>: SwiftUIAsset where M
decoder.logger?.t("Updating model for \(data.id)")
model.data = data
} else {
self.model = ModelType(data)
self.model = ModelType(data, userInfo: decoder.userInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update the userInfo when we get the model from the cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call

@@ -15,16 +15,21 @@ open class AssetViewModel<T: AssetData>: ObservableObject {
/// The decoded data
@Published public var data: T

/// Any contextual information set by the user
public var userInfo: [CodingUserInfoKey: Any]?
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't need to update this, we can make this let instead of var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kept it as var in order to set the userinfo when we update the model from cache

@nancywu1
Copy link
Contributor Author

not sure why the following tests InfoAssetUITests.testInfoBasic(), InputAssetUITests.testBasicInput(), InputAssetUITests.testInputValidation() keep failing for eyes not finding elements, but passes locally when i run them

@nancywu1 nancywu1 merged commit cbc2cfa into main Oct 26, 2023
1 check passed
@nancywu1 nancywu1 deleted the add-user-info-to-assetviewmodel branch October 26, 2023 15:26
@intuit-svc intuit-svc mentioned this pull request Oct 27, 2023
3 tasks
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.

2 participants