Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Adopt layout anchors #12147

Closed
wants to merge 2 commits into from
Closed

Adopt layout anchors #12147

wants to merge 2 commits into from

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Jun 14, 2018

Fixes #12144 and possibly #12143

Since we dropped support for iOS8, the layout anchor fluent API is now at our disposal. No more reaching out from MGLMapView to the containing app’s view controller to figure out how to layout the ornaments. This change also conforms to High Performance Auto Layout.

  • Write a blurb in the changelog
  • Adopt layout anchors for the diagnostic views

cc @fabian-guerra @1ec5 @julianrex @friedbunny

@julianrex
Copy link
Contributor

How does this PR affect @1ec5 's Reinstall constraints only when needed PR ?

Does it supersede it?

@friedbunny friedbunny added iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage refactor labels Jun 14, 2018
@friedbunny
Copy link
Contributor

friedbunny commented Jun 14, 2018

This seems very nice, thanks @frederoni. I think there are a couple points to consider:

  1. This removes MGLMapView.contentInset as a factor in these constraints, which I think is the right way to go, but it is a potentially significant change. Like @julianrex mentioned, we should consider whether or not Reinstall constraints only when needed #12144 (or a larger refactor) is needed.
  2. We’ve had a variety of regressions and issues with these constraints over the years and I’d worry about that happening again — are we happy with the test coverage of constraints/ornament layout or does this change require more tests?

@1ec5
Copy link
Contributor

1ec5 commented Jun 14, 2018

How does this PR affect @1ec5 's Reinstall constraints only when needed PR ?

The iOS part of #12144 would be superseded if the layout anchors account for the automaticallyAdjustsScrollViewInsets and contentInsetAdjustmentBehavior properties. But that seems unlikely, given that we’re manually mimicking UIScrollView – other UIView subclasses don’t need that functionality.

If we feel good about this PR, I’d suggest making the same changes to the macOS implementation of MGLMapView, to fully obviate #12144.

@julianrex
Copy link
Contributor

julianrex commented Jun 21, 2018

We should check this against #12053, #11491

@stale
Copy link

stale bot commented Oct 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the archived Archived because of inactivity label Oct 24, 2018
@friedbunny
Copy link
Contributor

This is still planned, but needs robust tests/testing before we can accept it.

@stale stale bot removed the archived Archived because of inactivity label Oct 24, 2018
@stale stale bot added the archived Archived because of inactivity label Dec 24, 2018
@stale
Copy link

stale bot commented Dec 24, 2018

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Dec 24, 2018
@friedbunny friedbunny reopened this Jan 2, 2019
@stale stale bot removed the archived Archived because of inactivity label Jan 2, 2019
@stale stale bot added the archived Archived because of inactivity label Mar 3, 2019
@stale
Copy link

stale bot commented Mar 3, 2019

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Mar 3, 2019
@fabian-guerra
Copy link
Contributor

Until #13911 lands will keep open this pr.

@fabian-guerra fabian-guerra reopened this Mar 4, 2019
@stale stale bot removed the archived Archived because of inactivity label Mar 4, 2019
@stale stale bot added the archived Archived because of inactivity label May 3, 2019
@stale
Copy link

stale bot commented May 3, 2019

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this May 3, 2019
@fabian-guerra
Copy link
Contributor

This was addressed in #13911

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived because of inactivity iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants