[BEAM-13662] [Playground] Support mutlifile examples #16857
Conversation
Add multifile value to the response to the frontend
# Conflicts: # playground/backend/internal/api/v1/api.pb.go
# Conflicts: # playground/api/v1/api.proto # playground/backend/internal/api/v1/api.pb.go # playground/backend/internal/cloud_bucket/precompiled_objects.go # playground/backend/internal/utils/precompiled_objects_utils.go # playground/backend/internal/utils/precompiled_objects_utils_test.go # playground/frontend/lib/api/v1/api.pb.dart # playground/frontend/lib/api/v1/api.pbjson.dart
Regenerate files
# Conflicts: # playground/backend/internal/api/v1/api.pb.go # playground/infrastructure/api/v1/api_pb2.py
Regenerate files
…' into BEAM-13662
There was a problem hiding this comment.
@ElessarST when running this locally should I be seeing the multifile icons? I am not seeing them.
| ), | ||
| Row( | ||
| children: [ | ||
| if (example.isMultiFile) |
There was a problem hiding this comment.
@ElessarST may we consider instead having a children list outside this widget and adding the MultifilePopoverButton widget if the example.isMultiFile condition passes? This may improve the readability. I had to read a few times to understand as it was an if statement inside an array.
There was a problem hiding this comment.
@damondouglas thanks, I moved actions to the different component for the better readability
@damondouglas Sorry, In the screenshot, I temporarily marked all examples as multifile since we don't have them for now. |
| @@ -46,6 +51,14 @@ class DescriptionPopover extends StatelessWidget { | |||
| ), | |||
| ), | |||
| Text(example.description), | |||
| if (example.link?.isNotEmpty ?? false) | |||
There was a problem hiding this comment.
I think it would improve the code readability to not have conditional logic in the array. This is just my subjective opinion and feel free to go your own way. I flag this as review and will accept/embrace your unchanging it however.
There was a problem hiding this comment.
@damondouglas I left to condition on the array, but I refactored it and I think it's now clear with the condition.
Wrap(
runSpacing: kMdSpacing,
children: [
title,
description,
if (hasLink) getViewOnGithub(context),
],
),
| } | ||
|
|
||
| Widget get multifilePopover { | ||
| return MultifilePopoverButton( |
There was a problem hiding this comment.
To improve readability, the use of => could possibly apply here.
| ); | ||
| } | ||
|
|
||
| Widget get descriptionPopover { |
There was a problem hiding this comment.
To improve readability, the use of => could possibly apply here.
There was a problem hiding this comment.
@pabloem LGTM
@ElessarST (cc: @ilya-kozyrev and @KhaninArtur ) later when the code becomes stable, should we think about documenting on the Flutter widgets?
|
LGTM |
I added:
Note: In the screenshot, I temporarily marked all examples as multifile since we don't have them for now.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
ValidatesRunnercompliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.