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

Update Deep link validation UI #6544

Merged
merged 55 commits into from
Nov 9, 2023
Merged

Conversation

hangyujin
Copy link
Contributor

@hangyujin hangyujin commented Oct 17, 2023

Add notifacation cards, coloum filters, etc

Hooked up with rpc.

Figma: https://www.figma.com/file/5zGvx0fvINUUWsTGZsdAKh/Flutter-deeplinking-in-DevTool?node-id=386%3A57755&mode=dev

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or there is a reason for not adding tests.

build.yaml badge

If you need help, consider asking for help on Discord.

@hangyujin hangyujin requested a review from a team as a code owner October 17, 2023 20:08
@hangyujin hangyujin requested review from CoderDake, chunhtai and a team and removed request for a team October 17, 2023 20:08
@hangyujin hangyujin changed the title Update Deep link validation ui Update Deep link validation UI Oct 17, 2023
Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

Done with another pass of comments. Fine for this PR since we are already so deep into review, but in the future, please try to break code up into smaller PRs so that is easier to review and iterate upon :) thanks!

required this.tableView,
required this.controller,
});
final ColumnData<LinkData> domain = DomainColumn(controller);
Copy link
Member

Choose a reason for hiding this comment

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

This is probably because the type is not specified in the columns parameter below:
columns: <ColumnData>[ ==> columns: <ColumnData<LinkData>>[

and then you should be able to remove the types here

required this.tableView,
required this.controller,
});
final ColumnData<LinkData> domain = DomainColumn(controller);
Copy link
Member

Choose a reason for hiding this comment

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

That and FlatTable should have a type too: FlatTable<LinkData>


import '../../shared/analytics/analytics.dart' as ga;
import '../../shared/analytics/constants.dart' as gac;
import '../../shared/config_specific/server/server.dart' as server;
import 'deep_links_model.dart';

const String _apiKey = 'AIzaSyDVE6FP3GpwxgS4q8rbS7qaf6cAbxc_elc';
const String _assetLinksGenerationURL =
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use store as URI directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably refactor out most of the http related thing into a file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do it in next pr as kenzie also mentioned this pr is getting too large

_packageNameKey: applicationId,
_domainsKey: [selectedLink.value!.domain],
// TODO(hangyujin): The fake fingerprints here is just for testing usage, should remove it later.
'supplemental_sha256_cert_fingerprints': [
Copy link
Contributor

Choose a reason for hiding this comment

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

do you know what would happen if not supplied and doesn't have play console project set up? I think most of our customer will use auto sign by play store. They won't be able to provide this finger prints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if not supplied and doesn't have play console project set up=> the http response will be error

Added a todo here to handle this error case

if (failedChecks != null) {
for (final Map<String, dynamic> failedCheck in failedChecks) {
switch (failedCheck[_checkNameKey]) {
case 'EXISTENCE':
Copy link
Contributor

Choose a reason for hiding this comment

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

should we turn this into const string?
also should probably move these into a file.

}

enum DomainError {
existence('Domain doesn\'t exist'),
Copy link
Contributor

Choose a reason for hiding this comment

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

what about other connectivity errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update them later

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM, if my concerns are tracked will be addressed in later prs

@hangyujin hangyujin added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2023
Copy link

auto-submit bot commented Nov 9, 2023

auto label is removed for flutter/devtools/6544, due to - The status or check suite Dart Code Metrics has failed. Please fix the issues identified (or deflake) before re-applying this label.

@hangyujin hangyujin merged commit a7d4cf6 into flutter:master Nov 9, 2023
19 checks passed
@hangyujin hangyujin deleted the deep-link-v2 branch February 12, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants