-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Improve code reuse in ChartsXAxis
and ChartsYAxis
#19198
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
[charts] Improve code reuse in ChartsXAxis
and ChartsYAxis
#19198
Conversation
Deploy preview: https://deploy-preview-19198--material-ui-x.netlify.app/ Bundle size report
|
import { isInfinity } from '../internals/isInfinity'; | ||
|
||
export const useAxisProps = (inProps: ChartsYAxisProps) => { | ||
export function useAxisTicksProps(inProps: ChartsYAxisProps) { |
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.
This hook is now only for ticks
* | ||
* - [ChartsYAxis API](https://mui.com/x/api/charts/charts-y-axis/) | ||
*/ | ||
function ChartsYAxis(inProps: ChartsYAxisProps) { |
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.
Moved most of the shared logic back to ChartsYAxis
. No need to abstract it into a hook if it's just used here.
Now ChartsSingleYAxis
and ChartsGroupedYAxis
are only responsible for rendering ticks.
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.
A lot of the logic can be kept in a hook IMO. Makes the component "cleaner"
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 agree that we can probably make this component cleaner, but I'm not sure if moving all the logic to a hook is the best way to do it. We'd have a big hook with many parameters returning an object with a bunch of values. That hides the complexity, but doesn't reduce it. I think a better way might be to separate the logic into smaller pieces, which I can try to do in a follow-up PR.
CodSpeed Performance ReportMerging #19198 will not alter performanceComparing Summary
|
b236920
to
5b49f62
Compare
ChartsYAxis
ChartsXAxis
and ChartsYAxis
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.
We should probably append Ticks
to ChartsGrouped*Axis
, ChartsGrouped*Axis
if we are only rendering the ticks
/* Gap between the axis label and tick labels. */ | ||
export const AXIS_LABEL_TICK_LABEL_GAP = 2; | ||
|
||
export const YAxisRoot = styled(AxisRoot, { |
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.
Might as well remove the XAxisRoot
from the x version, no?
* | ||
* - [ChartsYAxis API](https://mui.com/x/api/charts/charts-y-axis/) | ||
*/ | ||
function ChartsYAxis(inProps: ChartsYAxisProps) { |
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.
A lot of the logic can be kept in a hook IMO. Makes the component "cleaner"
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
5b49f62
to
37f9fe7
Compare
const drawingArea = useDrawingArea(); | ||
const { left, top, width, height } = drawingArea; |
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.
Looks weird
const drawingArea = useDrawingArea(); | |
const { left, top, width, height } = drawingArea; | |
const { left, top, width, height } = useDrawingArea(); |
Refactored
ChartsXAxis
andChartsYAxis
.The shared logic that was previous in
ChartsYAxis
was moved back into it.ChartsYAxis
is responsible for rendering the axis itself, the line and the label, and it also decides whether to renderChartsSingleYAxis
orChartsGroupedYAxis
.ChartsSingleYAxis
andChartsGroupedYAxis
now only render ticks according to their logic. The same logic applies to the x axis.