Skip to content
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

Refactoring the utilities to reduce monosize of components #31291

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

srmukher
Copy link
Contributor

@srmukher srmukher commented May 7, 2024

Refactoring the utilities to remove indirect function invocations. This reduced the monosize of each cartesian components by 4kB on an average.
image

@srmukher srmukher requested a review from a team as a code owner May 7, 2024 06:30
@srmukher srmukher changed the title Users/srmukher/refactor utils Refactoring the utilities to reduce monosize of components May 7, 2024
@fabricteam
Copy link
Collaborator

fabricteam commented May 7, 2024

📊 Bundle size report

✅ No changes found

Copy link

codesandbox-ci bot commented May 7, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@@ -424,7 +402,6 @@ export class CartesianChartBase extends React.Component<IModifiedCartesianChartP
yScale,
this.props.noOfCharsToTruncate || 4,
this.props.showYAxisLablesTooltip || false,
this.state.startFromX,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this argument was unused

@AtishayMsft
Copy link
Contributor

Can you update the PR description to represent only the changes in this PR.

@@ -970,180 +933,6 @@ describe('domainRageOfVerticalNumeric', () => {
});
});

describe('getDomainNRangeValues', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the test cases for the functions which were removed as the original functions were invoked. The tests for the original functions they call are still there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share the corresponding tests for the scenario that you have removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions that say 'getDomainNRangeValues' function called earlier were: domainRangeOfNumericForAreaChart
domainRangeOfVSBCNumeric
domainRageOfVerticalNumeric
domainRangeOfNumericForHorizontalBarChartWithAxis
domainRangeOfDateForAreaLineVerticalBarChart
domainRangeOfXStringAxis

All tests of these internal functions exist in the same file.

@srmukher srmukher force-pushed the users/srmukher/RefactorUtils branch from d2dc25f to d023c9b Compare May 15, 2024 13:06
@Thanneerudeepika
Copy link

Performed Full test pass on this PR https://fluentuipr.z22.web.core.windows.net/pull/31291/public-docsite/index.html#/controls.
Tested the functionality (manual testing) for all the charts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants