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

[BEAM-14310] [Website] bug home playground #17444

Merged
merged 4 commits into from Apr 28, 2022

Conversation

bullet03
Copy link
Contributor

[BEAM-14310]

  • add container to iframe
  • add js script to fix bug with scroll of iframe using css prop pointer-event
  • add styles to iframe

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

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)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@asf-ci
Copy link

asf-ci commented Apr 22, 2022

Can one of the admins verify this patch?

2 similar comments
@asf-ci
Copy link

asf-ci commented Apr 22, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Apr 22, 2022

Can one of the admins verify this patch?

@bullet03
Copy link
Contributor Author

R: @nausharipov

// License for the specific language governing permissions and limitations under
// the License.

$('#playgroundIframeContainer').on('click', function(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

'event' is declared but its value is never read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Copy link
Contributor

Choose a reason for hiding this comment

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

@bullet03 I think it's better to use underscore for unused params:

$('#playgroundIframeContainer').on('click', function(_) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
// License for the specific language governing permissions and limitations under
// the License.

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid future confusion, I suggest to give a more specific name to the file (something like "fix-playground-nested-scroll.js") and add a comment that describes its purpose.

Example of a comment:
"This script fixes the issue of a sudden scroll interruption when a user's cursor hovered over the embedded playground"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. gonna fix in future commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bullet03 why future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

allow="clipboard-write">
</iframe>
<div id="playgroundIframeContainer">
<iframe
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's format this file. Do it in a separate commit in case other reviewers ask us to revert the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was told to make less commits. before that i made a new one on each logical step.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bullet03 in this case it would make sense to commit the formatted code separately

@dariabezkorovaina
Copy link
Contributor

@pcoet this is a fix for scrolling issue - the scrolling was "stuck" when on Playground section, can you please review? :)

@dariabezkorovaina
Copy link
Contributor

@pabloem this PR was reviewed by David, can you please help us merge? :)

@pabloem
Copy link
Member

pabloem commented Apr 28, 2022

thanks!

@pabloem pabloem merged commit 9cbe78e into apache:master Apr 28, 2022
jrmccluskey pushed a commit to jrmccluskey/beam that referenced this pull request May 3, 2022
…playground

* [BEAM-14310] [Website] fix scroll bug on iframe

* [BEAM-14310] [Website] fix playground iframe scroll bug

* [BEAM-14310] [Website] delete unused params in fix-playground.js

* [BEAM-14310] [Website] changed name of fix-playground, added param with underscore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants