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 FXIOS-9627-[Native Error Page] Initial changes for manager support #22613

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tusharC95
Copy link
Collaborator

📜 Tickets

Jira ticket
Github issue

💡 Description

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@tusharC95 tusharC95 force-pushed the tc/FXIOS-9627-NativeErrorPage-Manager branch from 2cacdf5 to f7d4652 Compare October 24, 2024 12:55
configureUI()
setupLayout()
adjustConstraints()
showViewForCurrentOrientation()
}

// MARK: Redux
func newState(state: NativeErrorPageState) {
// if state.title != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

?? what dis?

@@ -89,37 +89,67 @@ final class NativeErrorPageViewController: UIViewController,

private var contraintsList = [NSLayoutConstraint]()

required init?(
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we're removing this?

}

static let reducer: Reducer<Self> = { state, action in
print(action.actionType)
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't have these statements in production


static let reducer: Reducer<Self> = { state, action in
print(action.actionType)
guard action.windowUUID == .unavailable || action.windowUUID == state.windowUUID else { return NativeErrorPageState(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return can be on a separate line here

title: state.title,
description: state.description
)}
switch action.actionType {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: leave some space so it's easier to read. Vertical space is free, we may as well take advantage!

)}
switch action.actionType {
case NativeErrorPageMiddlewareActionType.initialize:
guard let action = action as? NativeErrorPageAction, let model = action.nativePageErrorModel else {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spread the multiple let conditions over multiple lines for easy reading

Comment on lines +26 to +30
let newAction = NativeErrorPageAction(nativePageErrorModel: model,
windowUUID: windowUUID,
actionType: NativeErrorPageMiddlewareActionType.initialize
)
store.dispatch(newAction)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let newAction = NativeErrorPageAction(nativePageErrorModel: model,
windowUUID: windowUUID,
actionType: NativeErrorPageMiddlewareActionType.initialize
)
store.dispatch(newAction)
store.dispatch(
NativeErrorPageAction(nativePageErrorModel: model,
windowUUID: windowUUID,
actionType: NativeErrorPageMiddlewareActionType.initialize
)
)

nit: alternative to consider that doesn't require assignment.

final class NativeErrorPageMiddleware {
lazy var nativeErrorPageProvider: Middleware<AppState> = { state, action in
let windowUUID = action.windowUUID
print(action.actionType)
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 print statements in prod

lazy var nativeErrorPageProvider: Middleware<AppState> = { state, action in
let windowUUID = action.windowUUID
print(action.actionType)
switch action.actionType {
Copy link
Contributor

Choose a reason for hiding this comment

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

spppaaaaaaccccceeeee :P

func parseErrorDetails() -> ErrorPageModel {
var title = ""
var description = ""
switch error.code {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space for readibility

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