alexeyinkin commented on code in PR #23532: URL: https://github.com/apache/beam/pull/23532#discussion_r989836044
########## playground/frontend/lib/modules/examples/components/filter/filter.dart: ########## @@ -0,0 +1,97 @@ +/* + * 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:flutter_gen/gen_l10n/app_localizations.dart'; +import 'package:flutter/material.dart'; +import 'package:playground/constants/sizes.dart'; +import 'package:playground/modules/examples/components/examples_components.dart'; +import 'package:playground/pages/playground/states/example_selector_state.dart'; +import 'package:playground_components/playground_components.dart'; +import 'package:provider/provider.dart'; + +class ExamplesFilter extends StatelessWidget { + const ExamplesFilter({Key? key}) : super(key: key); Review Comment: `super.key` or no key. ########## playground/frontend/lib/modules/examples/components/filter/tag_bubble.dart: ########## @@ -0,0 +1,53 @@ +/* + * 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:flutter/material.dart'; +import 'package:playground/pages/playground/states/example_selector_state.dart'; +import 'package:playground_components/playground_components.dart'; +import 'package:provider/provider.dart'; + +class TagBubble extends StatelessWidget { + final String name; + + const TagBubble({ + Key? key, Review Comment: `super.key` or no key. ########## playground/frontend/lib/pages/playground/states/example_selector_state.dart: ########## @@ -22,27 +22,61 @@ import 'package:playground_components/playground_components.dart'; class ExampleSelectorState with ChangeNotifier { final PlaygroundController _playgroundController; ExampleType _selectedFilterType; - String _filterText; + String _searchText; List<CategoryWithExamples> categories; + List<String> tags = []; + List<String> selectedTags = []; ExampleSelectorState( this._playgroundController, this.categories, [ this._selectedFilterType = ExampleType.all, - this._filterText = '', - ]); + this._searchText = '', + ]) { + tags = _getTagsSortedByPopularity(categories); + } ExampleType get selectedFilterType => _selectedFilterType; - String get filterText => _filterText; + String get searchText => _searchText; void setSelectedFilterType(ExampleType type) { _selectedFilterType = type; notifyListeners(); } - void setFilterText(String text) { - _filterText = text; + void addSelectedTag(String tag) { + selectedTags.add(tag); + notifyListeners(); + } + + void removeSelectedTag(String tag) { + selectedTags.remove(tag); + notifyListeners(); + } + + List<String> _getTagsSortedByPopularity( + List<CategoryWithExamples> categories, + ) { + Map<String, int> tagsPopularity = {}; + for (var category in categories) { Review Comment: `final` here and below. ########## playground/frontend/playground_components/pubspec.lock: ########## @@ -0,0 +1,873 @@ +# Generated by pub Review Comment: Please delete this file. Libraries should not version-control `pubspec.lock`. I will soon add it to `.gitignore`. ########## playground/frontend/playground_components/lib/src/controllers/example_loaders/content_example_loader.dart: ########## @@ -37,6 +37,8 @@ class ContentExampleLoader extends ExampleLoader { name: descriptor.name ?? 'Embedded_Example', path: '', description: '', + // TODO(nausharipov): should tags be empty? Review Comment: Yes. You may delete this comment. ########## playground/frontend/lib/pages/playground/states/example_selector_state.dart: ########## @@ -22,27 +22,61 @@ import 'package:playground_components/playground_components.dart'; class ExampleSelectorState with ChangeNotifier { final PlaygroundController _playgroundController; ExampleType _selectedFilterType; - String _filterText; + String _searchText; List<CategoryWithExamples> categories; + List<String> tags = []; + List<String> selectedTags = []; ExampleSelectorState( this._playgroundController, this.categories, [ this._selectedFilterType = ExampleType.all, - this._filterText = '', - ]); + this._searchText = '', + ]) { + tags = _getTagsSortedByPopularity(categories); Review Comment: This is not popularity but example count. ########## playground/frontend/lib/modules/examples/components/filter/filter.dart: ########## @@ -0,0 +1,97 @@ +/* + * 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:flutter_gen/gen_l10n/app_localizations.dart'; +import 'package:flutter/material.dart'; +import 'package:playground/constants/sizes.dart'; +import 'package:playground/modules/examples/components/examples_components.dart'; +import 'package:playground/pages/playground/states/example_selector_state.dart'; +import 'package:playground_components/playground_components.dart'; +import 'package:provider/provider.dart'; + +class ExamplesFilter extends StatelessWidget { + const ExamplesFilter({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context) { + return Padding( + padding: const EdgeInsets.symmetric( + horizontal: kLgSpacing, + vertical: kMdSpacing, + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: const [ + _Types(), + _Tags(), + ], + ), + ); + } +} + +class _Types extends StatelessWidget { + const _Types({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context) { + AppLocalizations appLocale = AppLocalizations.of(context)!; + + return Row( + children: [ + TypeBubble( + type: ExampleType.all, + name: appLocale.all, + ), + TypeBubble( + type: ExampleType.example, + name: appLocale.examples, + ), + TypeBubble( + type: ExampleType.kata, + name: appLocale.katas, + ), + TypeBubble( + type: ExampleType.test, + name: appLocale.unitTests, + ), + ], + ); + } +} + +class _Tags extends StatelessWidget { + const _Tags({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context) { + return Consumer<ExampleSelectorState>( + builder: (context, state, child) => Padding( + padding: const EdgeInsets.symmetric(vertical: kMdSpacing), + child: SingleChildScrollView( Review Comment: Is there a reason to not use `ListView` here? ########## playground/frontend/lib/pages/playground/states/example_selector_state.dart: ########## @@ -51,56 +85,56 @@ class ExampleSelectorState with ChangeNotifier { notifyListeners(); } - void sortCategories() { + void filterCategoriesWithExamples() { final categories = _playgroundController.exampleCache.getCategories( _playgroundController.sdk, ); - - final sortedCategories = categories + final filteredCategories = categories .map((category) => CategoryWithExamples( title: category.title, - examples: _sortCategoryExamples(category.examples))) + examples: _filterExamples(category.examples))) .where((category) => category.examples.isNotEmpty) .toList(); - setCategories(sortedCategories); + setCategories(filteredCategories); + } + + List<ExampleBase> _filterExamples(List<ExampleBase> examples) { + final byType = filterExamplesByType(examples, selectedFilterType); + final byTags = filterExamplesByTags(byType); + final byName = filterExamplesByName(byTags); + return byName; } - List<ExampleBase> _sortCategoryExamples(List<ExampleBase> examples) { - final isAllFilterType = selectedFilterType == ExampleType.all; - final isFilterTextEmpty = filterText.isEmpty; - if (isAllFilterType && isFilterTextEmpty) { + List<ExampleBase> filterExamplesByTags(List<ExampleBase> examples) { Review Comment: `@visibleForTesting` here and below. ########## playground/frontend/lib/pages/playground/states/example_selector_state.dart: ########## @@ -22,27 +22,61 @@ import 'package:playground_components/playground_components.dart'; class ExampleSelectorState with ChangeNotifier { final PlaygroundController _playgroundController; ExampleType _selectedFilterType; - String _filterText; + String _searchText; List<CategoryWithExamples> categories; + List<String> tags = []; + List<String> selectedTags = []; ExampleSelectorState( this._playgroundController, this.categories, [ this._selectedFilterType = ExampleType.all, - this._filterText = '', - ]); + this._searchText = '', + ]) { + tags = _getTagsSortedByPopularity(categories); + } ExampleType get selectedFilterType => _selectedFilterType; - String get filterText => _filterText; + String get searchText => _searchText; void setSelectedFilterType(ExampleType type) { _selectedFilterType = type; notifyListeners(); } - void setFilterText(String text) { - _filterText = text; + void addSelectedTag(String tag) { + selectedTags.add(tag); + notifyListeners(); + } + + void removeSelectedTag(String tag) { + selectedTags.remove(tag); + notifyListeners(); + } + + List<String> _getTagsSortedByPopularity( + List<CategoryWithExamples> categories, + ) { + Map<String, int> tagsPopularity = {}; + for (var category in categories) { + for (var example in category.examples) { + for (var tag in example.tags) { + if (tagsPopularity[tag] != null) { Review Comment: `exampleCountByTag[tag] = (exampleCountByTag[tag] ?? 0) + 1` Or maybe without parentheses, check how precedence works. ########## playground/frontend/lib/pages/playground/states/example_selector_state.dart: ########## @@ -51,56 +85,56 @@ class ExampleSelectorState with ChangeNotifier { notifyListeners(); } - void sortCategories() { + void filterCategoriesWithExamples() { final categories = _playgroundController.exampleCache.getCategories( _playgroundController.sdk, ); - - final sortedCategories = categories + final filteredCategories = categories .map((category) => CategoryWithExamples( title: category.title, - examples: _sortCategoryExamples(category.examples))) + examples: _filterExamples(category.examples))) .where((category) => category.examples.isNotEmpty) .toList(); - setCategories(sortedCategories); + setCategories(filteredCategories); + } + + List<ExampleBase> _filterExamples(List<ExampleBase> examples) { + final byType = filterExamplesByType(examples, selectedFilterType); + final byTags = filterExamplesByTags(byType); + final byName = filterExamplesByName(byTags); + return byName; } - List<ExampleBase> _sortCategoryExamples(List<ExampleBase> examples) { - final isAllFilterType = selectedFilterType == ExampleType.all; - final isFilterTextEmpty = filterText.isEmpty; - if (isAllFilterType && isFilterTextEmpty) { + List<ExampleBase> filterExamplesByTags(List<ExampleBase> examples) { + if (selectedTags.isEmpty) { return examples; } - if (!isAllFilterType && isFilterTextEmpty) { - return sortExamplesByType( - examples, - selectedFilterType, - ); + List<ExampleBase> sorted = []; + for (var example in examples) { + if (example.tags.toSet().containsAll(selectedTags)) { + sorted.add(example); + } } - if (isAllFilterType && !isFilterTextEmpty) { - return sortExamplesByName(examples, filterText); - } - final sorted = sortExamplesByType( - examples, - selectedFilterType, - ); - return sortExamplesByName(sorted, filterText); + return sorted; } - List<ExampleBase> sortExamplesByType( + List<ExampleBase> filterExamplesByType( List<ExampleBase> examples, ExampleType type, ) { + if (type == ExampleType.all) { + return examples; + } return examples.where((element) => element.type == type).toList(); } - List<ExampleBase> sortExamplesByName( - List<ExampleBase> examples, - String name, - ) { + List<ExampleBase> filterExamplesByName(List<ExampleBase> examples) { + if (searchText.isEmpty) { Review Comment: `_searchText` here and below. ########## playground/frontend/lib/modules/examples/components/filter/filter.dart: ########## @@ -0,0 +1,97 @@ +/* + * 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:flutter_gen/gen_l10n/app_localizations.dart'; +import 'package:flutter/material.dart'; +import 'package:playground/constants/sizes.dart'; +import 'package:playground/modules/examples/components/examples_components.dart'; +import 'package:playground/pages/playground/states/example_selector_state.dart'; +import 'package:playground_components/playground_components.dart'; +import 'package:provider/provider.dart'; + +class ExamplesFilter extends StatelessWidget { + const ExamplesFilter({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context) { + return Padding( + padding: const EdgeInsets.symmetric( + horizontal: kLgSpacing, + vertical: kMdSpacing, + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: const [ + _Types(), + _Tags(), + ], + ), + ); + } +} + +class _Types extends StatelessWidget { + const _Types({Key? key}) : super(key: key); Review Comment: `super.key` or no key. ########## playground/frontend/lib/modules/examples/components/filter/filter.dart: ########## @@ -0,0 +1,97 @@ +/* + * 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:flutter_gen/gen_l10n/app_localizations.dart'; +import 'package:flutter/material.dart'; +import 'package:playground/constants/sizes.dart'; +import 'package:playground/modules/examples/components/examples_components.dart'; +import 'package:playground/pages/playground/states/example_selector_state.dart'; +import 'package:playground_components/playground_components.dart'; +import 'package:provider/provider.dart'; + +class ExamplesFilter extends StatelessWidget { + const ExamplesFilter({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context) { + return Padding( + padding: const EdgeInsets.symmetric( + horizontal: kLgSpacing, + vertical: kMdSpacing, + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: const [ + _Types(), + _Tags(), + ], + ), + ); + } +} + +class _Types extends StatelessWidget { + const _Types({Key? key}) : super(key: key); + + @override + Widget build(BuildContext context) { + AppLocalizations appLocale = AppLocalizations.of(context)!; + + return Row( + children: [ + TypeBubble( + type: ExampleType.all, + name: appLocale.all, + ), + TypeBubble( + type: ExampleType.example, + name: appLocale.examples, + ), + TypeBubble( + type: ExampleType.kata, + name: appLocale.katas, + ), + TypeBubble( + type: ExampleType.test, + name: appLocale.unitTests, + ), + ], + ); + } +} + +class _Tags extends StatelessWidget { + const _Tags({Key? key}) : super(key: key); Review Comment: `super.key` or no key. ########## playground/frontend/playground_components/lib/src/cache/example_cache.dart: ########## @@ -114,6 +114,8 @@ class ExampleCache extends ChangeNotifier { name: result.files.first.name, path: id, description: '', + // TODO(nausharipov): should tags be empty? Review Comment: Yes. You may delete this comment. ########## playground/frontend/test/pages/playground/states/example_selector_state_test.dart: ########## @@ -118,12 +118,30 @@ void main() { expect(state.categories, examplesSortedByTypeMock); expect(exampleCache.categoryListsBySdk, exampleCache.categoryListsBySdk); }); - state.sortExamplesByType(unsortedExamples, ExampleType.kata); + state.filterExamplesByType(unsortedExamples, ExampleType.kata); + }); + + // TODO(nausharipov): replace all 'sort' with 'filter' if ok Review Comment: Do replace please. ########## playground/frontend/playground_components/test/src/common/categories.dart: ########## @@ -18,14 +18,16 @@ import 'dart:collection'; -import 'package:playground_components/src/models/category_with_examples.dart'; -import 'package:playground_components/src/models/sdk.dart'; +import 'package:playground_components/playground_components.dart'; Review Comment: Please import specific files and not this bulk library. ########## playground/frontend/playground_components/test/src/common/categories.dart: ########## @@ -18,14 +18,16 @@ import 'dart:collection'; -import 'package:playground_components/src/models/category_with_examples.dart'; -import 'package:playground_components/src/models/sdk.dart'; +import 'package:playground_components/playground_components.dart'; import 'examples.dart'; final categoriesMock = [ CategoryWithExamples(title: 'Sorted', examples: [exampleMock1]), - CategoryWithExamples(title: 'Unsorted', examples: [exampleMock2]), + CategoryWithExamples( + title: 'Unsorted', + examples: [exampleMock1, exampleMock2, exampleMock2, exampleMock2], Review Comment: Are they repeated to test that 'tag2' is more frequent than 'tag1'? If so, explain in a comment please. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
