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

[web] Implement Link for web #3155

Merged
mdebbar merged 8 commits into from
Oct 20, 2020
Merged

[web] Implement Link for web #3155

mdebbar merged 8 commits into from
Oct 20, 2020

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Oct 14, 2020 •
edited
Loading

@mdebbar mdebbar requested a review from mklim as a code owner October 14, 2020 23:33
@google-cla google-cla bot added the cla: yes label Oct 14, 2020
@mdebbar mdebbar mentioned this pull request Oct 14, 2020
Merged
@mdebbar mdebbar removed the request for review from mklim October 14, 2020 23:34
@mdebbar mdebbar mentioned this pull request Oct 14, 2020
Closed
ditman
ditman reviewed Oct 15, 2020
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

This looks good to me; I'd move the link.dart file under src so users can't import it; that way I wouldn't worry much about making things _Private.

My only real concern would be to auto-applying a 'rel' attribute, or letting users set it alongside the 'target', so window.opener does not become an issue.

Everything else is nitpicking, and since most of this file is private anyways, it seems it can be refactored later, if needed/wanted.

ditman
ditman reviewed Oct 16, 2020
View reviewed changes
ditman
ditman approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Ship it, let's give this a shot!

@mdebbar mdebbar mentioned this pull request Oct 20, 2020
Closed
@mdebbar mdebbar requested a review from ditman October 20, 2020 19:06
ditman
ditman approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/url_launcher/url_launcher_web/test/test_driver/url_launcher_web_integration.dart
Comment on lines +236 to +237
group('link', () {
testWidgets('creates anchor with correct attributes',
Copy link
Member

@ditman ditman Oct 20, 2020

Choose a reason for hiding this comment

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

Thanks for the test! In a further PR, I think this should be separated to its own test file: link_integration.dart or similar.

Copy link
Contributor Author

@mdebbar mdebbar Oct 20, 2020

Choose a reason for hiding this comment

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

Yeah I want to also add more tests around hit testing but I couldn't make it work so it's gonna take some time. I wanted to land this ASAP to unbreak the url_launcher plugin.

@mdebbar mdebbar mentioned this pull request Oct 20, 2020
Merged
cyanglaz
cyanglaz approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@cyanglaz cyanglaz mentioned this pull request Oct 20, 2020
Merged
@mdebbar
Copy link
Contributor Author

mdebbar commented Oct 20, 2020

The red submit-queue is due to the breakage in url_launcher_platform_interface that this PR should fix.

Merging.

@mdebbar mdebbar merged commit 9d93e97 into flutter:master Oct 20, 2020
@ditman
Copy link
Member

ditman commented Oct 21, 2020

Thanks for publishing this, @mdebbar!

@ditman ditman mentioned this pull request Oct 21, 2020
Merged
@mdebbar mdebbar mentioned this pull request Oct 21, 2020
Merged
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@mdebbar
[web] Implement Link for web (flutter#3155)
6183e30
@neiljaywarner
Copy link

neiljaywarner commented Dec 31, 2020

So what version does this work in, is it working now?

@ditman
Copy link
Member

ditman commented Jan 5, 2021

@neiljaywarner This should work all the way up to flutter channel beta?

@neiljaywarner
Copy link

neiljaywarner commented Jan 5, 2021 •
edited
Loading

@ditman will it get documentation on flutter.dev or similar "soon"?

Also I might have had an issue where I didn't switch to most recent version bc nullsafety issue

@ditman
Copy link
Member

ditman commented Jan 5, 2021

@neiljaywarner there is documentation for the widget, here:

https://pub.dev/documentation/url_launcher/latest/link/Link-class.html

(The widget comes in the url_launcher plugin)

@stenlee
Copy link

stenlee commented May 5, 2022

Unfortunately, the CMD+Left Mouse Click still not working to open the link in the new tab as users are used in the "classic" web app.

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

Reviewers

@harryterkelsen harryterkelsen Awaiting requested review from harryterkelsen

2 more reviewers

@ditman ditman ditman approved these changes

@cyanglaz cyanglaz cyanglaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[url_launcher_web] class 'UrlLauncherPlugin' is missing implementations

5 participants

@mdebbar @ditman @neiljaywarner @stenlee @cyanglaz