[WCM] Implement missing ui + fix ui glitches #943
Conversation
radeknovis
changed the title
radeknovis
force-pushed
the
feature/wcm-cleanup
branch
from
8607019
to
d6b39ef
Compare
| @@ -31,3 +31,5 @@ DerivedDataCache | |||
| *.ipa | |||
| *.zip | |||
| test_results/ | |||
|
|
|||
| Sources/WalletConnectModal/Secrets/secrets.json | |||
There was a problem hiding this comment.
Where it used? Other team also need to have this file locally?
There was a problem hiding this comment.
Only needed if you want to run WalletConnectModal previews. If you don't have it, you will get a somewhat explanatory error to create it.
| dependencies: ["QRCode", "WalletConnectSign"]), | ||
| dependencies: ["QRCode", "WalletConnectSign"], | ||
| exclude: ["Secrets/secrets.json.sample"], | ||
| resources: [.copy("Secrets/secrets.json")] |
There was a problem hiding this comment.
Strange. You're ignoring this file by git, but it's required for WalletConnectModal SDK
There was a problem hiding this comment.
It is ignored, so we don't commit actual secrets. This will pass if the file is not present, but to access the JSON with actual secrets, you need to bundle it with the target.
| @@ -34,7 +33,7 @@ struct ModalContainerView_Previews: PreviewProvider { | |||
|
|
|||
| init() { | |||
|
|
|||
| let projectId = "9bfe94c9cbf74aaa0597094ef561f703" | |||
| let projectId = Secrets.load().projectID | |||
There was a problem hiding this comment.
Now we're taking Project ID from configuration. Why json file is better?
There was a problem hiding this comment.
configuration.xcconfig is not available from package, only for stuff in the ExampleApp project, or at least I haven't figured out how to read from it.
I'm not too keen on having two approaches how to share secrets/vars either. So any suggestions on how to do this better are more than welcome :D
There was a problem hiding this comment.
you can pass projectID to w3m in a config method like in Relay
static public func configure(
relayHost: String = "relay.walletconnect.com",
projectId: String,
socketFactory: WebSocketFactory,
socketConnectionType: SocketConnectionType = .automatic
)
There was a problem hiding this comment.
can't believe we missed comiting proj id xd
There was a problem hiding this comment.
Yeah, this is how it will be done when actually integrating the modal SDK https://github.com/WalletConnect/WalletConnectSwiftV2/blob/9cead7d4443815687e02cd87ba888130225283fc/Sources/WalletConnectModal/WalletConnectModal.swift#L51-L57C8
but this is in the context of previews within SDK itself so I don't have any projectID injected from elsewhere.
| @@ -34,3 +34,15 @@ struct Listing: Codable, Hashable, Identifiable { | |||
| let universal: String? | |||
| } | |||
| } | |||
|
|
|||
| #if DEBUG | |||
There was a problem hiding this comment.
I'd move all stubs in separate folder "Stubs". To not pollute SDK code
| @@ -12,7 +12,7 @@ struct AsyncImage<Content>: View where Content: View { | |||
|
|
|||
| var request = URLRequest(url: url) | |||
| request.setValue(ExplorerAPI.userAgent, forHTTPHeaderField: "User-Agent") | |||
| request.setValue(WalletConnectModal.config.metadata.name, forHTTPHeaderField: "Referer") | |||
| request.setValue(WalletConnectModal.config?.metadata.name, forHTTPHeaderField: "Referer") | |||
There was a problem hiding this comment.
Could you double check that it sets normal string instead of Optional("...")
There was a problem hiding this comment.
There is a unit test for this, verifying that both User-Agent and Referer headers are correctly populated.
|
@flypaper0 I've tried to address all your comments. Can you have a look again, please? |
radeknovis
force-pushed
the
feature/wcm-cleanup
branch
from
7b34984
to
65ae0ba
Compare
No description provided.