Fix missing labels on certain axes and label filter configurations #47563
Conversation
flash1293
added
Feature:XYAxis
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
flash1293
changed the title
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
| @@ -105,15 +106,22 @@ export class AxisLabels { | |||
| selection.selectAll('.tick text') | |||
| .text(function (d) { | |||
| const par = d3.select(this.parentNode).node(); | |||
There was a problem hiding this comment.
nit: renaming this to const parentNode bring a bit more readability
💔 Build Failed |
💔 Build Failed |
…xis-tick-orientation
|
Weird, that doesn't happen locally. I will merge master and run again, if the error persists I will dig in. |
💔 Build Failed |
…xis-tick-orientation
|
The test that's failing isn't even hitting the code I changed and I don't get that failure locally. @dmlemeshko do you have an idea what the problem could be here? The number of ticks changes based on the height of the chart, and it seems like the browser window is taller on CI than locally (based on measuring the screenshot in the jenkins job). Is this a known problem? Is there a workaround? |
💔 Build Failed |
Sorry for the late reply. This problem is known and there are other tests adjusted to pass on CI and still be failing locally. I don't like that approach since local failures are confusing. A quick solution is to adjust expected results to pass on both local & CI, a better one is to convert tests to visual regression |
…xis-tick-orientation
…xis-tick-orientation
…xis-tick-orientation
|
Wow, that took way too long. @markov00 could you take another look? |
rayafratkina
added
release_note:fix
and removed
release_note:skip
|
@flash1293 One issue with the chart size on Jenkins vs locally is that the tests run on Jenkins with headless Chrome. The command I think it will take a bit of testing to figure out the right size setting to use to get the tests all passing with the existing expected results, but they should stay consistent in all cases when done. |
|
@LeeDr right, I remember now we discussed that. After syncing with @markov00: However as the rest of these suites are still flaky and fixing that would go beyond the scope of this PR (which is fixing the bug with missing tick labels on rotating the chart), we decided to split that part out and resolve it separately - there is already an issue for that: #22322 I added a non-skipped functional test to just check for the bug to prevent a regression. I also left in some fixes to the page objects that were necessary because the UI changed but went unnoticed because the tests are skipped. Those will be helpful when tackling #22322 in a separate PR. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Fixes #32059
The root cause is that the old filter code assumed that the ticks are traversed from low position numbers to high position numbers, but that's not always the case (not 100% sure why, maybe due to the way to how we use d3).
This PR makes the existing implementation more robust by calculating the start and end edge for each tick and then checking whether they overlap with the last tick placed. This will work for all cases where the ticks are traversed in order (either from left to right or from right to left)