-
Notifications
You must be signed in to change notification settings - Fork 8.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
Introduce workaround for vega height bug #31461
Introduce workaround for vega height bug #31461
Conversation
💔 Build Failed |
cdfcf71
to
daf4590
Compare
💚 Build Succeeded |
Pinging @elastic/kibana-app |
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'm ok with this change, but is there a way to unit test this behavior? It would make it easier to remove this hack at a later date.
b6bad88
to
94efec0
Compare
@nyurik Done! It seems like this bug is triggered by very specific circumstances. For instance it didn't trigger if the font-size in the sample spec is smaller than 14px (maybe has sth. to do with the total height? No idea...) |
💚 Build Succeeded |
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.
Tested that it fixes the behavior. While testing figured out, that vega is really buggy with handling those signals in general. Hopefully something that will be fixed in future versions.
@timroes anything specific that Vega team can act on? They have been making amazing progress in code quality improvements - merged it into a monorepo, began using async/await, etc. It would be great if we can help them in this "never ending quest for quality and performance" :) |
Waiting for #31693 before merging this |
Tested the backported fix on 7.0, works also in IE |
* Failing test: Jest Tests.src/plugins/vis_type_vega/public * remove workaround for vega height bug Related to #31461 Co-authored-by: Elastic Machine <[email protected]>
) * Failing test: Jest Tests.src/plugins/vis_type_vega/public * remove workaround for vega height bug Related to elastic#31461 Co-authored-by: Elastic Machine <[email protected]>
…73015) * Failing test: Jest Tests.src/plugins/vis_type_vega/public * remove workaround for vega height bug Related to #31461 Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Fixes #25855
This is a workaround which hopefully becomes obsolete with vega 5 (currently upgrading doesn't work because of #31413
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately