-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[Website] add playground section #21959
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
[Website] add playground section #21959
Conversation
|
Can one of the admins verify this patch? |
4 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
d2940d0 to
9d1bc9f
Compare
d3a8b6e to
3723b77
Compare
website/www/site/layouts/index.html
Outdated
| </a> | ||
| <div id="playgroundIframeContainer"> | ||
| <iframe | ||
| src="https://play.beam.apache.org/embedded?enabled=true&example=SDK_JAVA/PRECOMPILED_OBJECT_TYPE_EXAMPLE/MinimalWordCount&code=" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to pass code if it's empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| class="code-snippet playground" | ||
| allow="clipboard-write"> | ||
| </iframe> | ||
| <div id="playgroundIframeContainer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id should not be used in shortcode because the shortcode can be used multiple times on a page. It actually will be used many times since we need to convert many doc highlighted examples to playground instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
website/www/site/layouts/index.html
Outdated
| <a class="playground__mobile" href="https://play.beam.apache.org/"> | ||
| <img src="images/playground.png" alt="beam playground"> | ||
| </a> | ||
| <div id="playgroundIframeContainer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a shortcode here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I understood according to the answer, we are not able to use shortcodes: https://discourse.gohugo.io/t/shortcode-inside-define-block/39410
| </iframe> | ||
| <div id="playgroundIframeContainer"> | ||
| <iframe | ||
| src="https://frontend-beta-dot-apache-beam-testing.appspot.com/embedded?enabled={{ .Get 0 }}&example={{ .Get 1 }}&code={{ .Get 2 }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use named parameters in this shortcode since we already have 5. They will only be adding up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opportunity to update embedded playground source to https://play.beam.apache.org/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| ## Beam Playground WordCount Example | ||
|
|
||
| {{< playground "true" "SDK_JAVA/PRECOMPILED_OBJECT_TYPE_EXAMPLE/MinimalWordCount" "" "700px">}} | ||
| {{< playground "true" "SDK_JAVA/PRECOMPILED_OBJECT_TYPE_EXAMPLE/MinimalWordCount" "" "700px" "playgroundIframe">}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have another vision for this. We often will need Playground with tabs for multiple languages. A shortcode should be like
{{< playground height="700px" >}}
{{< playground_snippet sdk="java" example="SDK_JAVA/PRECOMPILEDOBJECT_TYPE_EXAMPLE/MinimalWordCount" ... >}}
{{< playground_snippet sdk="python" ... >}}
{{< /playground >}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that playground_snippet inserts a div with data-xxx attributes or something like this.
playground shortcode then inserts a JS that looks for these nested dives. If only one is given, it renders a single iframe. If multiple are given, it creates tabs.
If you need to push this fast, can you rename playground shortcode to playground_iframe or something like so we can reserve playground shortcode for the above solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed playground shortcode to playground_iframe
| src="https://frontend-beta-dot-apache-beam-testing.appspot.com/embedded?enabled={{ .Get 0 }}&example={{ .Get 1 }}&code={{ .Get 2 }}" | ||
| width="100%" | ||
| height="{{ .Get 3 }}" | ||
| id="{{ .Get 4 }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use for this ID? Is it used outside of the shortcode? If not, can it be generated here or omitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playground now has subdomain https://play.beam.apache.org/ - would be nice to update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
e6b7c47 to
51be88d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to add and remove classes to the div instead if tweaking styles at runtime. I suggest to:
- Rename this class to
playground-iframe-wrapper-no-scrollso it is self-descriptive. (Dashes are for this to go in line with my parallel work on embedding). - On click, add another class
playground-iframe-wrapper-scrollthat setspointer-events: autoon iframe so it takes over. - On mouse leave, remove
playground-iframe-wrapper-scrollclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
playground/frontend/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is hard to read without an example. I think I will add it myself when I'm done on my part.
Sorry for the overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed Embedding note from README
|
I renamed I did it in this branch because it is likely the fastest way to deploy it, and we want this change before URLs start to spread. |
1e197af to
2692f9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done on $(this), otherwise we affect all Playground instances on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to include this on each page where this scrolling trick is used? If so, it is inconvenient.
I see the following options to avoid it:
-
There seems to be a technique to check if a shortcode is used anywhere in the page to include scripts in that case:
https://gohugo.io/templates/shortcode-templates/#checking-for-existence -
Inline JS in the shortcode. It may be non-trivial if we want to avoid IDs on iframes and divs.
-
Include this JS unconditionally on all pages as the last resort, since Playground will likely be used on most of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, used approach #1.
Made a partial from this code, so it could be reusable in future, since playground will be used on many pages.
{{ if .HasShortcode "playground_iframe" }}
{{ $fixPlaygroundNestedScroll := resources.Get "js/fix-playground-nested-scroll.js" | minify | fingerprint }}
<script type="text/javascript" src="{{ $fixPlaygroundNestedScroll.RelPermalink }}" defer></script>
{{ end }}
10ae5c0 to
3dfa455
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only adds this check to get-started, but not the home page. Maybe better add this to footer.html? That will cover all pages on the website.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to head.html/head_homepage.html
5ad7835 to
d9f3fdf
Compare
alexeyinkin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
d9f3fdf to
87bced5
Compare
alexeyinkin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
87bced5 to
62a0d0a
Compare
|
Known limitations:
This PR will also expose the trade-off from #22154 (comment) that was chosen to fix #22153. It was not visible before because the embedded Playground was read-only since the last deployment and will become editable again in this PR. |
|
@alexeyinkin Thank you for adding notes from our testing and triage with @alevtinaboiko and @bullet03 |
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pabloem LGTM
Resolves #22301
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.