Skip to content

Rebuild InvalidStorageViewController with Texture#1109

Merged
tomholub merged 3 commits intomasterfrom
feature/issue-761
Nov 28, 2021
Merged

Rebuild InvalidStorageViewController with Texture#1109
tomholub merged 3 commits intomasterfrom
feature/issue-761

Conversation

@Kharchevskyi
Copy link
Contributor

@Kharchevskyi Kharchevskyi commented Nov 28, 2021

This PR rebuild InvalidStorageViewController with Texture. Added UI Guidance

close #766

IMG_E723E2654C50-1

Tests:

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

This is an interesting example - I find that the UIKit example was more readable and overall more straight forward. From this perspective, it may make sense to stop writing new controllers in Texture, and start migrating away from it.

I wonder what would it look like in SwiftUI.

@Kharchevskyi
Copy link
Contributor Author

What if we would need to add one new element for example some extra button between views. We would need to relayout all views accordingly.
With UIKit tableview this would be the same as in this PR but with instead uitableview and manual size calculations

@Kharchevskyi
Copy link
Contributor Author

Kharchevskyi commented Nov 28, 2021

I would prefer to move towards swift ui instead of ui kit

@Kharchevskyi
Copy link
Contributor Author

Kharchevskyi commented Nov 28, 2021

Initially ui was built by just adding uiviews instead of table view usage, which I think less flexible and reusable.

Also ui will be much slower and less smooth on uikit, due to main thread only usage. And what is much more important - all size calculations for ui elements (for example cell height based on text) should be done manually. This would lead to completely reworking compose flow or any other flow with dynamic height

@tomholub
Copy link
Collaborator

Of course we should not be converting from Texture to UIKit, that would be a downgrade.

What would this view look like when implemented in SwiftUI? Could you take a look?

@Kharchevskyi
Copy link
Contributor Author

This is just one example. For full understanding try for example to rewrite To UIKit - ComposeViewController where not only text cell should calculate size but also recipients part.

@Kharchevskyi
Copy link
Contributor Author

Sure. Should I try Swift ui in this pr or may I open another issue for this?

@tomholub
Copy link
Collaborator

Actually, now is not the best time. In the future, we'll look at SwiftUI separately. So we could proceed with this PR as is.

)
)
case .description:
return SetupTitleNode(
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like if we have very long stack trace text here, we will have very long node (big height) and button will be hidden from user. User have to scroll down to see button. In my original version button is always on screen, independent of stack trace node height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can keep this behaviour, but should we?
Apple Human Interface Guidelines recommends to use table views for scrollable layout. Also it's more common for iOS users to scroll if content is higher than screen size. And it's quite rare to have such long localized description

Copy link
Collaborator

Choose a reason for hiding this comment

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

The exception that caused us to put this in place was indeed quite long, causing a long scroll.

I guess it's fine if the button ends up on the bottom, where user has to scroll. It's more user friendly when it's on top, but it's not too bad on a screen that should be rarely, if ever, seen.

@tomholub tomholub enabled auto-merge (squash) November 28, 2021 22:08
@tomholub tomholub merged commit 72bdc0d into master Nov 28, 2021
@tomholub tomholub deleted the feature/issue-761 branch November 28, 2021 23:05
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.

storage error screen uses UIKit - guidance with ADK

3 participants