Fixes #39380 - Update line chart to PatternFly5#612
Conversation
tlabaj
force-pushed
the
linechart
branch
2 times, most recently
from
bacaf52 to
a6bd34f
Compare
Lukshio
left a comment
Lukshio
left a comment
There was a problem hiding this comment.
Hi, I know it's WIP, but just first round of review.
|
Hi, please commit new changes in new commits. |
Sure, I will do that going forward. |
| import { | ||
| formatAxisTick, | ||
| formatTooltipTitle, | ||
| formatYAxisTick, | ||
| getLegendEvents, | ||
| getSeriesOpacity, | ||
| getXAxisTickValues, | ||
| InteractiveLegendLabel, | ||
| InteractiveLegendSymbol, | ||
| XAxisTickLabel, | ||
| } from 'foremanReact/components/common/charts/AreaChart/AreaChartLegend'; |
There was a problem hiding this comment.
Can you please open new PR in foreman core and move all imports to new /chars/helpers/LegendHelpers.js so it can be easily reused later? So it won't be matched to specific chart. The AreaChartLegend will then use new helper.
Path update will be needed after the change.
There was a problem hiding this comment.
@Lukshio Here is the Foreman PR theforeman/foreman#11010
|
@tlabaj ForemanCore PR is merged, you can edit import paths |
Friendly reminder: If a dependency in Foreman is required and was merged, please update https://github.com/theforeman/foreman_openscap/blob/master/lib/foreman_openscap/engine.rb#L43 accordingly |
Lukshio
left a comment
Lukshio
left a comment
There was a problem hiding this comment.
Code changes looks good, please update the engine.rb to new foreman version, and fix CI failures
I updated the |
@tlabaj Redmine is OK, but linter erros has be fixed. |
|
If it's going to be merged after linter fix, please squash before or on merge. |
|
Not a blocker, but a question: I've seen Foreman core already has a LineChart component (https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/components/common/charts/LineChart/index.js#L19), but this PR adds another one specifically for openscap. Why's that? Wouldn't it be better to migrate the core's one to PF5 and then re-use it here?.. |
|
@ofedoren We prefer using PF directly and through wrappers, and matching each component use to the format of data it receives. From what I saw, trying to create wrappers that will match different types of needs and data, resulted in complicated code to re-use, read and update. (disclaimer: I did a quick look at the code only). |
|
QA-wise LGTM. Results After: One small nitpick (non-blocking): while using float for the Y-axis is fine-ish (WRT scaling and such), using it in the tooltip with Passed/Failed/Othered rules looks a bit strange, as a rule is non-divisible. |
|
@tlabaj could you please change the float numbers? |
|
Re-verified with the latest PackIt build. The float numbers are fixed. Everything else looks also fine 👍 
|
5 participants
Fixes: #39380
This PR updates the Line Chart to use PF5 components.
Before:
After:
