-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix rte scroll handler firing on child scroll #430
Conversation
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.
Some unrelated changes seem to have got in (possibly related to your restructuring work), but the relevant changes look good.
@@ -9,7 +9,6 @@ | |||
"test:watch": "jest --watch", | |||
"build": "NODE_ENV=production webpack --config webpack.prod.js", | |||
"build:scripts": "NODE_ENV=production webpack --config webpack.cli.js", | |||
"prepublish": "in-publish && npm run build || not-in-publish", |
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.
These changes don't look related to this PR.
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 actually did add this on purpose, I'll update the PR subject so that's clear.
@@ -68,7 +67,6 @@ | |||
"file-loader": "^0.8.5", | |||
"identity-obj-proxy": "^3.0.0", | |||
"imports-loader": "^0.6.5", | |||
"in-publish": "^2.0.0", |
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.
These changes don't look related to this PR.
@@ -1,5 +1,7 @@ | |||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. |
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.
These changes don't look related to this PR.
@@ -81,7 +81,9 @@ export class StickyContext extends Component { | |||
} | |||
|
|||
handleScroll = (event) => { | |||
this.updateStickies(event.target); | |||
if (event.target === this.ref) { |
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
@Benaiah have another look, I removed prepublish on purpose. |
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
LGTM |
- Summary
Code blocks can scroll horizontally in the rich text editor, but those scrolls were
propagating and triggering the scroll handler for the StickyContext component,
causing the toolbar to stick to the top of the scroll context.
This fix ensures that only context scrolls trigger the handler.
Also, fixed an issue where the recently added in-publish script fails CI.