Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Sep 22, 2020

Description
Currently addPlatformView's offset parameter is not used for iOS/Android. This PR clarifies that offset is not used for in addPlatformView for iOS and Android and removes offset from the scenario_app test for iOS/Android platform views.

Not yet removing offset completely since it may be used for Fuschia/Web. It may be desirable to remove offset completely for consistency between the platforms.

Also updated outdated comment stating addPlatformView is only supported for iOS.

Related Issues

flutter/flutter#37029

Tests

I added the following tests:
N/A

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@RichardJCai RichardJCai force-pushed the remove_offset_from_platform_view_hidden branch from 86704b3 to 0aa2de4 Compare September 22, 2020 20:20
for Android and iOS and remove offset from scenario_app test for iOS and Android.
@RichardJCai RichardJCai force-pushed the remove_offset_from_platform_view_hidden branch from 0aa2de4 to ecb9c35 Compare September 22, 2020 21:42
@RichardJCai RichardJCai changed the title remove unused offset parameter from _addPlatformView private methods clarify that offset is not used in addPlatformView for iOS/Android Sep 22, 2020
@RichardJCai RichardJCai marked this pull request as ready for review September 22, 2020 22:36
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants