Updating demos and hacking fix for responsive container #84
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amber-eb
reviewed
Dec 26, 2017
| if (window.requestAnimationFrame) { | ||
| window.requestAnimationFrame(runCallbacks); | ||
| } else { | ||
| setTimeout(runCallbacks, 66); |
There was a problem hiding this comment.
Magic number? Where did this come from?
There was a problem hiding this comment.
Good call, moving into a constant.
| let newWidth = document.querySelector('main').offsetWidth; | ||
|
|
||
| if (cachedWidth !== newWidth) { | ||
| cachedWidth = newWidth; |
There was a problem hiding this comment.
Would it be better to put this inside the !running condition?
| @@ -48,6 +48,24 @@ const optimizedResize = (function() { | |||
| } | |||
| }; | |||
|
|
|||
| const resizeMainHorizontal = () => { | |||
| let newWidth = document.querySelector('main').offsetWidth; | |||
There was a problem hiding this comment.
I'm not familiar enough with the codebase to know what main is. Is that something that's always guaranteed to be present?
There was a problem hiding this comment.
That's the element of the 'main' section of the documentation.
It is not safe that it will always be there, and I cannot find how to add a class there to make it easier.
adding a comment.
amber-eb
approved these changes
Dec 26, 2017
davegomez
added a commit
to davegomez/britecharts-react
that referenced
this pull request
Dec 5, 2018
The resizeMainHorizontal was added in the PR britecharts#84 as a hacking fix for the responsive container in the project documentation. This function broke the charts responsiveness outside the docs because it was looking for a `main` tag present in the styleguide docs but not in every other project.
miglesiasEB
pushed a commit
that referenced
this pull request
Dec 6, 2018
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> The resizeMainHorizontal was added in the PR #84 as a hacking fix for the responsive container in the project documentation. This function broke the charts responsiveness outside the docs because it was looking for a `main` tag present in the **React Styleguidist** docs but not in every other project. This PR removes the problematic function and uses the correct one to fix the problem. The doc's responsiveness was tested using the original function and it work ok with this version of React Styleguidist. This PR also address the issue #144 to make it easier to import the ResponsiveContainer component and the withResponsiveness helper. ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here. --> * The charts' responsiveness is currently broken as described in the issue #105. * At the moment is difficult to import the ResponsiveContainer component and the withResponsiveness helper because you have to do it directly from the ESM bundle. ## How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> * All the unit tests were ran and all of them passed. * The documentation pages were reviewed to check for any inconsistency with the published one. ## Screenshots (if appropriate): <img width="1676" alt="wide" src="https://user-images.githubusercontent.com/282903/49551467-5007dc80-f8bd-11e8-9611-53de3d8166c9.png"> <img width="1237" alt="narrow" src="https://user-images.githubusercontent.com/282903/49551475-58601780-f8bd-11e8-9fd6-eaf84ecaf96e.png"> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Refactor (changes the way we code something without changing its functionality) - [x] 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] I have read the **CONTRIBUTING** document. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [x] All new and existing tests passed.
miglesiasEB
pushed a commit
that referenced
this pull request
May 31, 2019
* Updating demos and hacking fix for responsive container * Amber comments
miglesiasEB
pushed a commit
that referenced
this pull request
May 31, 2019
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> The resizeMainHorizontal was added in the PR #84 as a hacking fix for the responsive container in the project documentation. This function broke the charts responsiveness outside the docs because it was looking for a `main` tag present in the **React Styleguidist** docs but not in every other project. This PR removes the problematic function and uses the correct one to fix the problem. The doc's responsiveness was tested using the original function and it work ok with this version of React Styleguidist. This PR also address the issue #144 to make it easier to import the ResponsiveContainer component and the withResponsiveness helper. ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here. --> * The charts' responsiveness is currently broken as described in the issue #105. * At the moment is difficult to import the ResponsiveContainer component and the withResponsiveness helper because you have to do it directly from the ESM bundle. ## How Has This Been Tested? <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> * All the unit tests were ran and all of them passed. * The documentation pages were reviewed to check for any inconsistency with the published one. ## Screenshots (if appropriate): <img width="1676" alt="wide" src="https://user-images.githubusercontent.com/282903/49551467-5007dc80-f8bd-11e8-9611-53de3d8166c9.png"> <img width="1237" alt="narrow" src="https://user-images.githubusercontent.com/282903/49551475-58601780-f8bd-11e8-9fd6-eaf84ecaf96e.png"> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Refactor (changes the way we code something without changing its functionality) - [x] 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] I have read the **CONTRIBUTING** document. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [x] All new and existing tests passed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Patching the layout resizing issue by programmatically trigger a window resize event
Update legend demo to something a bit more nice
Motivation and Context
the overflow hidden trick didn't really work, as it just masked the problem.
I tried to debounce the application of the responsive container, but it breaks the functionality after the first trigger.
How Has This Been Tested?
Ran tests
Types of changes