You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "Malarg (via GitHub)" <gi...@apache.org> on 2023/02/10 07:24:50 UTC
[GitHub] [beam] Malarg commented on a diff in pull request #24957: [ToB] [Frontend] Tour of Beam analytics
Malarg commented on code in PR #24957:
URL: https://github.com/apache/beam/pull/24957#discussion_r1102258523
##########
learning/tour-of-beam/frontend/lib/modules/analytics/events.dart:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY IND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class TobAnalyticsEvents {
Review Comment:
all these variables are not used. Suggest not insert to pr dead code
##########
learning/tour-of-beam/frontend/lib/modules/analytics/categories.dart:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY IND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class TobAnalyticsCategories {
Review Comment:
all these variables are not used. Suggest not insert to pr dead code
##########
learning/tour-of-beam/frontend/lib/pages/tour/state.dart:
##########
@@ -29,7 +29,9 @@ import '../../cache/unit_progress.dart';
import '../../config.dart';
import '../../models/unit.dart';
import '../../models/unit_content.dart';
+// import '../../modules/analytics/google_analytics_service.dart';
Review Comment:
delete?
##########
playground/frontend/playground_components/lib/src/services/analytics/google_analytics4_non_web_service.dart:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:meta/meta.dart';
+
+import 'events/abstract.dart';
+import 'google_analytics4_service.dart';
+
+@internal
+GoogleAnalytics4Service createGoogleAnalytics4Service({
+ required String propertyId,
+}) =>
+ GoogleAnalytics4NonWebService();
+
+/// The required placeholder for non-web builds, e.g. unit tests.
+class GoogleAnalytics4NonWebService extends GoogleAnalytics4Service {
+ GoogleAnalytics4NonWebService() : super.create();
+
+ @override
+ Future<void> sendProtected(AnalyticsEvent event) {
+ throw UnimplementedError();
Review Comment:
why unimplemented?
##########
playground/frontend/playground_components/lib/src/services/analytics/google_analytics4_web_service.dart:
##########
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+import 'dart:convert';
+import 'dart:html'; // ignore: avoid_web_libraries_in_flutter
Review Comment:
lets remove this rule?
##########
playground/frontend/lib/modules/actions/components/new_example_action.dart:
##########
@@ -38,7 +38,9 @@ class NewExampleAction extends StatelessWidget {
label: 'intents.playground.newExample'.tr(),
onPressed: () {
launchUrl(Uri.parse('/'));
- AnalyticsService.get(context).trackClickNewExample();
+ PlaygroundComponents.analyticsService.sendUnawaited(
Review Comment:
`PlaygroundComponents.analyticsService.sendNewExampleEvent`
##########
learning/tour-of-beam/frontend/lib/modules/analytics/events.dart:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY IND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class TobAnalyticsEvents {
+ static const openUnit = 'open_unit'; // datetime?
Review Comment:
suppose this comments should be more clear to understand
##########
playground/frontend/lib/components/playground_run_or_cancel_button.dart:
##########
@@ -20,38 +20,38 @@ import 'package:flutter/widgets.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import '../modules/analytics/analytics_service.dart';
-import '../utils/analytics_utils.dart';
-
class PlaygroundRunOrCancelButton extends StatelessWidget {
const PlaygroundRunOrCancelButton();
@override
Widget build(BuildContext context) {
return Consumer<PlaygroundController>(
- builder: (context, playgroundController, child) {
- final analyticsService = AnalyticsService.get(context);
- final stopwatch = Stopwatch();
- final exampleName = getAnalyticsExampleName(playgroundController);
-
- return RunOrCancelButton(
- playgroundController: playgroundController,
- beforeCancel: () {
- final exampleName = getAnalyticsExampleName(playgroundController);
- analyticsService.trackClickCancelRunEvent(exampleName);
- },
- beforeRun: () {
- stopwatch.start();
- analyticsService.trackClickRunEvent(exampleName);
- },
- onComplete: () {
- analyticsService.trackRunTimeEvent(
- exampleName,
- stopwatch.elapsedMilliseconds,
- );
- },
- );
- }
+ builder: (context, playgroundController, child) => RunOrCancelButton(
+ playgroundController: playgroundController,
+ beforeCancel: (runner) {
+ PlaygroundComponents.analyticsService.sendUnawaited(
Review Comment:
Lets make this interface more laconic?
Suggest make it like this: `PlaygroundComponents.analyticsService.sendCancelRun`, which will call `sendUnawaited` with `CancelRunAnalyticsEvent` param
##########
playground/frontend/lib/pages/standalone_playground/widgets/feedback/feedback_dropdown_content.dart:
##########
@@ -40,11 +39,15 @@ class FeedbackDropdownContent extends StatelessWidget {
static const sendButtonKey = Key('sendFeedbackButtonKey');
final void Function() close;
+ final EventContext eventContext;
+ final FeedbackRating feedbackRating;
final TextEditingController textController;
const FeedbackDropdownContent({
Key? key,
Review Comment:
`super.key`, or remove
##########
playground/frontend/integration_test/miscellaneous_ui/enjoy_playground_test.dart:
##########
@@ -58,14 +56,18 @@ Future<void> _checkEnjoyingAndSendFeedback(WidgetTester wt) async {
await wt.tap(find.feedbackDropdownSendButton());
await wt.pumpAndSettle();
- final context = wt.element(find.feedbackThumbUp());
- final lastSentEvent = AnalyticsService.get(context).lastSentEvent;
+ final lastEvent = PlaygroundComponents.analyticsService.lastEvent;
expect(
- lastSentEvent,
- AnalyticsEvent(
- category: kFeedbackCategory,
- action: kClickSendFeedbackEvent,
- label: text,
+ lastEvent,
+ const FeedbackFormAnalyticsEvent(
Review Comment:
lets implements DRY principle. Suggest extract this to a method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org