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

[ABW-1750] MFA: list and create securitystructures from settings #580

Merged

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Jun 12, 2023

Jira ticket: ABW-1750

Adds ability to list and create SecurityStructureConfigurations from settings
Also make it possible to display Profile using raw JSON

Video

trim.4189F22F-D07F-4393-A26C-CF483E85A6DC.MOV

@CyonAlexRDX CyonAlexRDX requested a review from kugel3 June 12, 2023 14:50
@CyonAlexRDX CyonAlexRDX changed the title Mfa/abw 1750 list and create securitystructures from settings [ABW-1750] MFA: list and create securitystructures from settings Jun 12, 2023
let xOffset: CGFloat = 50
JSONView(jsonString: json)
.frame(
width: geoProxy.frame(in: .global).width + xOffset,
Copy link
Contributor

@kugel3 kugel3 Jun 12, 2023

Choose a reason for hiding this comment

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

What is the goal with GeometryReader here? It looks like it makes the JSON view 50 pixels wider than its container and moves it 50 pixels left? If that is what you really want to do you could probably just use a negative padding.

Copy link
Contributor Author

@CyonAlexRDX CyonAlexRDX Jun 13, 2023

Choose a reason for hiding this comment

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

Nevermind, got it working without GeometryReader now

@kugel3 the goal is not the offset (good idea, I did not think about using padding, that might make be be able to remove the xOffset completely, thanks for the suggestion!) - the goal is to make this view expand. I really hope there is a better solution, and would be great if you can tell me :D I gave up after like 15 minutes, trying fixedSize trying layoutPriority trying setting infinte CGRect frame on the UIKit view (JSONView wraps a dependency, being a UIView), I tried setting resizing masks, I tried changing translatesAutoresizingMasksIntoConstraints... and then felt, fudge it, not worth the hassle, since this is just for debug.

But I'm still curious if there is a SwiftUI solution I missed which will make it possible to remove the GeometryReader...

(actually the proper solution regarding the padding is to Fork the library and make it possible to remove/hide the 50 points wide line number view, which we dont care about and which eats up precious width, but I deemed it not worth it given that this is only for debug)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I think I got it working now :P by... doing... nothing. strange! something in the parent view must have changed..

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you shouldn't really have to do anything here.

Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

LGTM!
some minor suggestions

public var body: some SwiftUI.View {
WithViewStore(store, observe: \.viewState, send: { .view($0) }) { viewStore in
VStack {
Text("\(viewStore.label)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think viewStore.label can be used directly without wrapping in double quotes.

}

// MARK: - SecurityStructureConfigListCoreView
public struct SecurityStructureConfigListCoreView: View {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this could be just a function instead of a new struct, matter of taste :).

public func reduce(into state: inout State, childAction: ChildAction) -> EffectTask<Action> {
switch childAction {
case let .config(id, action: .delegate(.displayDetails)):
guard let config = state.configs[id: id]?.config else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the delegate action could also contain the config, so this check is not needed.

}
}

public func reduce(into state: inout State, viewAction: ViewAction) -> EffectTask<Action> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to add appeared action if it is not reduced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remnant of template, thx!

…ttings' of github.com:radixdlt/babylon-wallet-ios into MFA/ABW-1750_list_and_create_securitystructures_from_settings
@@ -15,6 +15,7 @@ public struct JSONView: SwiftUI.View {
public var body: some View {
#if canImport(UIKit)
UIKitJSONView(jsonString: jsonString)
.padding([.leading], -60) // we hide the "line number" view on the left which eats up precious widdth,zoo
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for brackets, it's an OptionSet

@@ -26,15 +26,9 @@ extension DebugInspectProfile {

public var body: some SwiftUI.View {
WithViewStore(store, observe: \.viewState, send: { .view($0) }) { viewStore in
GeometryReader { geoProxy in
ZStack {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the ZStack affect the layout? It shouldn't be needed otherwise, since if let ... else is a legitimate view builder pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-06-13 at 11 47 56

Resolved using Group instead. I was 50-50 between both. Don't know if there is any advantage with the one over the other..? Group is probably more performant/lightweight? not sure if it has drawbacks

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, didn't see the .toolbar, then it makes sense. I would prefer Group since we're not using z here. Or put the main bit in a func core(viewStore: ...).

@CyonAlexRDX CyonAlexRDX merged commit f663f86 into main Jun 13, 2023
@CyonAlexRDX CyonAlexRDX deleted the MFA/ABW-1750_list_and_create_securitystructures_from_settings branch June 13, 2023 10:39
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants