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

[web] Introduce the Link widget #2813

Closed
wants to merge 2 commits into from

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Jun 3, 2020

@mdebbar mdebbar changed the title Support Link using platform views [web] Introduce the Link widget Oct 13, 2020
@mdebbar mdebbar marked this pull request as ready for review October 13, 2020 22:45
@mdebbar mdebbar requested a review from mklim as a code owner October 13, 2020 22:45
@mdebbar mdebbar requested review from ditman and harryterkelsen and removed request for mklim October 13, 2020 22:45
Comment on lines +77 to +85
// TODO(mdebbar): how do we know if this is true or false?
final bool isUsingRouter = false;
// QUESTION: does this update the history entry on the engine side?
final MethodCall call = isUsingRouter
? MethodCall('pushRouteInformation', <dynamic, dynamic>{
'location': routeName,
'state': null,
})
: MethodCall('pushRoute', routeName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chunhtai is there a way currently to tell if the app is in Router mode? Maybe inspect the widget hierarchy for a Router widget?

Copy link

@chunhtai chunhtai Oct 14, 2020

Choose a reason for hiding this comment

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

there is no standard API to check that. Since this is a separate package, i think just do a Router.of(context, nullOk: true) != null should be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@override
Future<void> clearFocus() async {
// Currently this does nothing on Flutter Web.
// TODO(het): Implement this. See https://github.com/flutter/flutter/issues/39496
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was copied from the similar implementation in the framework. Not sure what's the right thing to do here @hterkelsen.

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

Code LGTM. Unfortunate that we have to use PlatformViewLink here for a pretty simple use case. This highlights how much we need to flesh out/fix HtmlElementView.

In order to actually land this, you will need to split it up into 2 CLs: one which lands the changes in the _interface package (with CHANGELOG and pubspec version bump) and one which uses those changes in package:url_launcher.

widget.link.isDisabled ? null : _followLink,
),
Positioned.fill(
// child: _LinkHtmlView(uri: link.uri, target: link.target),
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this line and add a TODO to use a PlatformView once hit testing is fixed and creation parameters supported

@mdebbar
Copy link
Contributor Author

mdebbar commented Oct 14, 2020

Closing this in favor of the split PRs: #3154 and #3155.

@mdebbar mdebbar closed this Oct 14, 2020
@ditman
Copy link
Member

ditman commented Oct 15, 2020

(Moved my review to the PRs above)

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

Successfully merging this pull request may close these issues.

5 participants