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.

Sign up for GitHub

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

Jump to bottom

Linechart triggering JS error when combined with responsive container #105

Closed
miglesiasEB opened this issue Jan 12, 2018 · 4 comments
Closed

Linechart triggering JS error when combined with responsive container #105

miglesiasEB opened this issue Jan 12, 2018 · 4 comments
Labels
bug docs help wanted needs reproducing
Projects
Release 1.0

Comments

@miglesiasEB
Copy link
Collaborator

Expected Behavior

No errors in console

Current Behavior

The line chart hover triggers errors in the console

Steps to Reproduce (for bugs)

  1. Add Responsive container to linechart demos
  2. Hover over it
  3. See error in console

fullscreen_1_11_18__5_27_pm

Your Environment

  • Browser Name and version: Chrome
@miglesiasEB miglesiasEB added this to Backlog in Release 1.0 Jan 12, 2018
@davegomez
Copy link
Contributor

davegomez commented Oct 3, 2018 •
edited

I got the same error and display when changed the topicLabel value from "topics" to something else in the <Line /> component prop.

So you should be good if you keep the text as "topics".

@davegomez
Copy link
Contributor

@Golodhros can you check if this issue still exist in the new version? I think this might be solved with the changes to the way we are binding the references.

@Golodhros
Copy link
Collaborator

Yeah, just added it to my list!

This was referenced Dec 4, 2018
Closed
Merged
miglesiasEB pushed a commit that referenced this issue Dec 6, 2018
@davegomez @miglesiasEB
Fix charts' responsiveness (#145)
0b73502 
<!--- 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 miglesiasEB mentioned this issue Dec 11, 2018
Merged
4 tasks
@miglesiasEB
Copy link
Collaborator Author

This is working properly too!
🎉

@miglesiasEB miglesiasEB moved this from Backlog to Done in Release 1.0 Dec 11, 2018
miglesiasEB pushed a commit that referenced this issue May 31, 2019
@davegomez @Golodhros
Fix charts' responsiveness (#145)
f62d0f7 
<!--- 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
Assignees
No one assigned
Labels
bug docs help wanted needs reproducing
Projects
Release 1.0
Done
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Golodhros @davegomez @miglesiasEB