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

Switch the gallery view to use a StackView instead of manual layout #542

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

theospears
Copy link
Collaborator

@theospears theospears commented Dec 5, 2024

Summary

Previously we used SnapKit and lots of constraints in order to control the layout of the gallery view. This was a bunch of code, and cause constraint violation warnings due to margins being impossible in conjunction with hiding elements by setting their height to 0.

Here we migrate to use UIStackView which automatically handles constraints, sizing, and removing hidden views. This lets us delete a lot of code, and also fixes a bug where there would be a rendering glitch if returning to the gallery view from a goal view where the keyboard was hidden - previously the area the keyboard had occupied would appear blank.

Known issues to fix before merging: The background for last updated, deadbeat, etc, do not use the full width of the screen.

Validation

Loaded the app on iPhone and iPad

  • Checked the layout looks good in portrait and landscape
  • Checked with and without keyboard
  • Checked navigating from goal view back to gallery view
  • Confirmed with the keyboard visible you can still scroll all goals

Copy link
Collaborator Author

theospears commented Dec 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@theospears theospears changed the title Switch to using uistack view Switch the gallery view to use a StackView instead of manual layout Dec 5, 2024
@krugerk
Copy link
Contributor

krugerk commented Dec 6, 2024

Yes Please! More UIStackView and less SnapKit. On our way toward SwiftUI.

Two more things for the list of known issues:

  • In the simulator, with the search bar present and the soft keyboard toggled on, tapping search off and back on, we see the last fetched and probably rest of the stack view coming in from the top of the device, passing through the safe area, passing through the navbar, on their way to being placed just under the navigation bar.
  • After signing in, the gallery is shown and initially has 0 goals while it fetches some. The 'you have no goals' message is shown quite low on the screen, much lower than it was before. Maybe it can be placed further up, after all, it is the only thing in the stack view, or in the middle, or we can re-check hint for no goal when filtering and ptr on empty list #506

@krugerk
Copy link
Contributor

krugerk commented Dec 6, 2024

and also fixes a bug where there would be a rendering glitch if returning to the gallery view from a goal view where the keyboard was hidden - previously the area the keyboard had occupied would appear blank.

and in the meantime, #545

Comment on lines +23 to +24
var stackView = UIStackView()
var collectionContainer = UIView()
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
var stackView = UIStackView()
var collectionContainer = UIView()
let stackView = UIStackView()
let collectionContainer = UIView()

@@ -56,15 +69,12 @@ class GalleryViewController: UIViewController, UICollectionViewDelegateFlowLayou
let item = UIBarButtonItem(image: UIImage(systemName: "gearshape.fill"), style: UIBarButtonItem.Style.plain, target: self, action: #selector(self.settingsButtonPressed))
self.navigationItem.rightBarButtonItem = item

self.view.addSubview(self.lastUpdatedView)
stackView.addArrangedSubview(self.lastUpdatedView)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about configuring each individual view up here and then afterwards, toward the bottom of this method, setting the arranged subviews all at once? I think it would increase readability by decreasing the scope needed to see which views are in the stackView and in which order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open to it, seems like a candidate for a follow up change.

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