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

add: initial dimension to ResponsiveContainer #3596

Merged
merged 2 commits into from May 23, 2023

Conversation

akamfoad
Copy link
Contributor

@akamfoad akamfoad commented May 21, 2023

Description

ResponsiveContainer component is used to render charts that can adapt to their container. When a chart is rendered inside Responsive container, you can omit its own height and width, and instead specify the dimensions on the Container, one of the dimensions should be a percentage, using % symbol, for instance 50%, recharts then tries to to render the chart accordingly. This is possible because when the component renders in the browser, then it'll calculate the container's dimensions, and then it'll calculate the width (or the height) that was specified in percentage.

When this ResponsiveContainer renders initially, its internal sizes are sepcified as { width: -1, height: -1 }, this is because it needs to render in the browser before it can calculate the dimentions. Also, ResponsiveContainer doesn't render the chart initialy since the dimentions are negative, so it'll try to find a positive height and width and render the chart accordingly.

This is important, because it means that, ResponsiveContainer relies on JavaScript and Browser functionalities to calculate its children size, and initially, it doesn't render the children. This can be a limitation when the ResponsiveContainer functionality is needed but the charts are rendered in an environment which is not the browser or JavaScript is not available.

Related Issue

Fixes #3595

Motivation and Context

In this commit, I'm changed this behavior such that, initial height and initial width can be specified to calculate the initial dimensions of the ResponsiveContainer's child. These props, if not provided, they have the original sizes (-1, -1) respectively, so this changes can be applied without breaking semver.

Its important to note that, these two props should be either provided or omitted together, because if either width or height negative, then ResponsiveContainer will render null until it'll calculate the dimensions on the client (Browser) after hydration.

Which means, its possible to render the charts in specified dimensions while rendering on server, and initial render on browser (before hydration), and when hydrated, the sizes will be calculated as before.

How Has This Been Tested?

Its possible to test this in any framework that is capable to render on server, and on client, but its also possible to enable and disable JavaScript. I chose Remix to test this since disabling and enabling is easily done by rendering or not rendering a component.

Use this example and comment out the <Scripts /> on line 28 to see how the chart looks when its server rendered but not hydrated.

Currently, it should render nothing when <Scripts /> is not rendered.

When this changes applied, it should render with inital dimensions.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

ResponsiveContainer component is used to render charts that can adapt to
their container. When a chart is rendered inside Responsive container,
you can omit its own height and width, and instead specify the
dimensions on the Container, one of the dimensions should be a
percentage, using `%` symbol, for instance 50%, recharts then tries to
to render the chart accordingly. This is possible because when the
component renders in the browser, then it'll calculate the container's
dimensions, and then it'll calculate the width (or the height) that was
specified in percentage.

When this ResponsiveContainer renders initially, its internal sizes are
sepcified as `{ width: -1, height: -1 }`, this is because it needs to
render in the browser before it can calculate the dimentions. Also,
ResponsiveContainer doesn't render the chart initialy since the
dimentions are negative, so it'll try to find a positive height and
width and render the chart accordingly.

This is important, because it means that, ResponsiveContainer relies on
JavaScript and Browser functionalities to calculate its children size,
and initially, it doesn't render the children. This can be a limitation
when the ResponsiveContainer functionality is needed but the
charts are rendered in an environment which is not the browser
or JavaScript is not available.

In this commit, I'm changed this behavior such that, initial height and
initial width can be specified to calculate the initial dimentions of
the ResponsiveContainer's child. These props, if not provided, they
have the original sizes (-1, -1) respectively, so this changes can be
applied without breaking semver.

Which means, its possible to render the charts in specified dimention
while rendering on server, and inital render on browser (before
hydration), and when hydrated, the sizes will be calculated as
before.
Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Just one suggestion - what do you think?

Comment on lines 26 to 27
initalWidth?: number;
initalHeight?: number;
Copy link
Member

Choose a reason for hiding this comment

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

could we pass these as an object where the object is optional but both properties height and width are required if specified? This way we can never have the height/width problem. You could make this initialDimentions?

Not terribly important but it does allow the API to say "if you want to set one of these you must set the other"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree its a better expression of the intended behavior. I changed it.

Using an optional `initialDimension`, that when provided should
include both `width` and `height` which is a better expression of
the API.
@akamfoad akamfoad changed the title add: initial width & height to ResponsiveContainer add: initial dimension to ResponsiveContainer May 22, 2023
Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

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

thanks for the changes 🚀

@ckifer
Copy link
Member

ckifer commented May 23, 2023

@akamfoad can you add an issue or make the changes needed to add this to the docs as well? In storybook is fine

@ckifer ckifer merged commit 1890285 into recharts:master May 23, 2023
5 checks passed
@akamfoad akamfoad mentioned this pull request May 24, 2023
9 tasks
ckifer pushed a commit that referenced this pull request May 30, 2023
## Description
Add ResponsiveContainer story and a simple example of how to use it.

## Related Issue

Follow up of #3596

## Motivation and Context

Document ResponsiveContainer and how its used.

## How Has This Been Tested?

## Screenshots (if appropriate):

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [X] Documentation & Storybook
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [X] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [X] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
renovate bot added a commit to SAP/ui5-webcomponents-react that referenced this pull request Jun 15, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [recharts](https://togithub.com/recharts/recharts) | [`2.6.2` ->
`2.7.0`](https://renovatebot.com/diffs/npm/recharts/2.6.2/2.7.0) |
[![age](https://badges.renovateapi.com/packages/npm/recharts/2.7.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/recharts/2.7.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/recharts/2.7.0/compatibility-slim/2.6.2)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/recharts/2.7.0/confidence-slim/2.6.2)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>recharts/recharts</summary>

###
[`v2.7.0`](https://togithub.com/recharts/recharts/releases/tag/v2.7.0)

[Compare
Source](https://togithub.com/recharts/recharts/compare/v2.6.2...v2.7.0)

#### What's Changed

Storybook updates, a few new features, bug fixes.

**Note:** This release may cause more reports of [this defaultProps
warning](https://togithub.com/recharts/recharts/issues/3615) - we are
aware of this and trying to come up with a feasible solution.

##### Feat

- Allow adding initial dimensions to ResponsiveContainer by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3596
- Extend clip path configuration with `clipDot` prop by
[@&#8203;tylerben](https://togithub.com/tylerben)
[@&#8203;ckifer](https://togithub.com/ckifer) in
[recharts/recharts#3602
- Support "bumpX" and "bumpY" curve types by
[@&#8203;jacknevitt](https://togithub.com/jacknevitt) in
[recharts/recharts#3617

##### Fix

- Account for `angle` prop on XAxis visibility calculation by
[@&#8203;bernardobelchior](https://togithub.com/bernardobelchior) in
[recharts/recharts#3576
- Propagate className to CartesianAxis by
[@&#8203;mitrotasios](https://togithub.com/mitrotasios) in
[recharts/recharts#3592
- Add SVGProps to PieLabel type by
[@&#8203;timbonicus](https://togithub.com/timbonicus) in
[recharts/recharts#3594
- Export default tooltip and legend content components by
[@&#8203;oschwede](https://togithub.com/oschwede) in
[recharts/recharts#3604
- Fix error bars not working in stacked bar charts by
[@&#8203;ckifer](https://togithub.com/ckifer) in
[recharts/recharts#3612
- Remove role="img" attribute from bar to prevent accessibility issues
by using incorrect role by
[@&#8203;rhuangabrielsantos](https://togithub.com/rhuangabrielsantos) in
[recharts/recharts#3614

##### Docs

- lots of storybook changes - [go check it
out](https://release--63da8268a0da9970db6992aa.chromatic.com/)!

##### Refactor

- refactor: `Dot` to function component by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3478
- refactor: `Polygon` to function component by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3479
- refactor: `Reactangle` to function component by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3480
- refactor: `Sector` to function component by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3481
- refactor: `Trapezoid` to function component by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3482
- refactor: change `Symbols` to functional component by
[@&#8203;Yilun-Sun](https://togithub.com/Yilun-Sun) in
[recharts/recharts#3485
- refactor: `DefaultTooltipContent` to be functional component by
[@&#8203;akamfoad](https://togithub.com/akamfoad) in
[recharts/recharts#3618

#### New Contributors

- [@&#8203;CoffeeGeek101](https://togithub.com/CoffeeGeek101) made their
first contribution in
[recharts/recharts#3561
- [@&#8203;bernardobelchior](https://togithub.com/bernardobelchior) made
their first contribution in
[recharts/recharts#3576
- [@&#8203;mitrotasios](https://togithub.com/mitrotasios) made their
first contribution in
[recharts/recharts#3592
- [@&#8203;timbonicus](https://togithub.com/timbonicus) made their first
contribution in
[recharts/recharts#3594
- [@&#8203;oschwede](https://togithub.com/oschwede) made their first
contribution in
[recharts/recharts#3604
- [@&#8203;rhuangabrielsantos](https://togithub.com/rhuangabrielsantos)
made their first contribution in
[recharts/recharts#3614
- [@&#8203;jacknevitt](https://togithub.com/jacknevitt) made their first
contribution in
[recharts/recharts#3617

**Full Changelog**:
recharts/recharts@v2.6.2...v2.7.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/SAP/ui5-webcomponents-react).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTcuMyIsInVwZGF0ZWRJblZlciI6IjM1LjExNy4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResponsiveContainer doesn't let me render charts on server or when JavaScript is disabled
2 participants