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

[Insets] Setup migration for accompanist/insets #1034

Merged
merged 5 commits into from
Mar 9, 2022
Merged

Conversation

alexvanyo
Copy link
Collaborator

@alexvanyo alexvanyo commented Feb 19, 2022

An initial PR beginning the process of deprecating accompanist/insets in favor of built-in androidx.compose.foundation support.

Most of the accompanist/insets APIs have been deprecated, with ReplaceWiths where possible. There are cases where ReplaceWith doesn't exactly match the new behavior and where complete auto-migration isn't possible (parameters became specific types), so this was a best-effort to help along the way.

The insets doc has been updated with a migration guide, along with a more complete table of migrations and an explanation of behavior differences.

There are some things that are not yet supported in androidx:

  • the IME animation controller has been left un-deprecated.
  • the components in insets-ui still exist, and I've added some variations that will help those work with the androidx.compose.foundation insets, and I am tracking upstreaming those if possible.

TODO:

  • Depend on non-snapshot version of Compose

@alexvanyo alexvanyo force-pushed the av/deprecated-insets branch 2 times, most recently from fe591f4 to 4ef088e Compare February 22, 2022 21:20
@@ -28,6 +30,7 @@ import androidx.compose.runtime.setValue
* offsets which describe changes to the four edges of a rectangle.
*/
@Stable
@Deprecated("accompanist/insets is deprecated")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be better in cases without a replaceWith to mention something like "it has been superseded by ..." so the user at least knows where to go and look?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, I updated the deprecated descriptions to briefly explain the equivalents of methods and classes that don't have exact equivalents, and also linked to the migration doc.

@@ -14,6 +14,8 @@
* limitations under the License.
*/

@file:Suppress("DEPRECATION")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please leave a comment on the sample uses of this with something like "to be replaced in a later PR to test the migration"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't planning to publish those in-place to the repo, although I have a branch with those changes here working off of the migrations:
https://github.com/google/accompanist/tree/av/deprecated-insets-replace-samples

@alexvanyo alexvanyo marked this pull request as ready for review February 25, 2022 18:41
@alexvanyo
Copy link
Collaborator Author

@bentrengrove and @simona-anomis this is ready for another look!

Copy link
Collaborator

@bentrengrove bentrengrove left a comment

Choose a reason for hiding this comment

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

This LGTM! Just the one comment, wondering if we should write the docs first before merging this and pointing people to docs that don't exist.

"""
accompanist/insets is deprecated.
The androidx.compose equivalent of LocalWindowInsets is the extensions on WindowInsets.
For more migration information, please visit https://google.github.io/accompanist/insets/#migration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to publish those docs first before merging this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not entirely sure what the doc publishing process is, will those go out immediately after publishing?

If so, the updated docs/insets.md in this PR should make this link real after the merge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry! I completely missed that you had changed it because GitHub had collapsed it

@alexvanyo alexvanyo merged commit b9a44ab into main Mar 9, 2022
@alexvanyo alexvanyo deleted the av/deprecated-insets branch March 9, 2022 16:23
@alexvanyo
Copy link
Collaborator Author

Thanks @bentrengrove and @simona-anomis !

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.

None yet

3 participants