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

Replaced window resize event handler with resizeObserver #1839

Merged
merged 5 commits into from
Aug 27, 2023

Conversation

currents-jantu
Copy link
Contributor

@currents-jantu currents-jantu commented Jan 31, 2023

This is a known bug so, not creating an issue.
This changes makes this component to work with collapsible sidebar, basically it will takes care of RGL width change even if window is not resized

@github-actions github-actions bot added the core use this label for changes in `lib` directory label Jan 31, 2023
@STRML
Copy link
Collaborator

STRML commented Feb 24, 2023

I'm not married to IE11 compatibility, but this will break it. What's the issue corresponding to this?

@currents-jantu
Copy link
Contributor Author

currents-jantu commented Feb 24, 2023

Thank you for your response. I apologize for not creating an issue earlier. I will ensure to create one promptly to keep track of this.

Regarding Pull Request #984, I believe your suggestion to use ResizeSensor makes more sense in this context.

@STRML
Copy link
Collaborator

STRML commented Jun 23, 2023

Please rebase this @currents-jantu and I will merge.

@STRML
Copy link
Collaborator

STRML commented Jun 23, 2023

Ok. This breaks tests due to the missing ResizeObserver global. I would prefer we use the ResizeSensor instead or something that will not break tests.

@currents-jantu
Copy link
Contributor Author

currents-jantu commented Jun 23, 2023

Ok. This breaks tests due to the missing ResizeObserver global. I would prefer we use the ResizeSensor instead or something that will not break tests.

Sure, it make sense

@github-actions github-actions bot added deps use this label for changes in the project's dependencies infrastructure labels Jun 27, 2023
@STRML STRML merged commit 2d72f8b into react-grid-layout:master Aug 27, 2023
@STRML
Copy link
Collaborator

STRML commented Aug 27, 2023

@currents-jantu I am now realizing this broke the resize tests. Could you please fix it?

 FAIL  test/spec/lifecycle-test.js
  ● Lifecycle tests › <ReactGridLayout> › WidthProvider › Renders with WidthProvider measureBeforeMount

    expect(received).toEqual(expected) // deep equality

    Expected: "ReactGridLayout"
    Received: "div"

      318 |
      319 |         // Should have removed the div, now has the RGL
    > 320 |         expect(wrapper.childAt(0).childAt(0).name()).toEqual("ReactGridLayout");
          |                                                      ^
      321 |         expect(wrapper.childAt(0).state().width).toEqual(500);
      322 |       });
      323 |

      at Object.toEqual (test/spec/lifecycle-test.js:320:54)

  ● Lifecycle tests › <ReactGridLayout> › WidthProvider › WidthProvider responds to window resize events

    expect(received).toEqual(expected) // deep equality

    Expected: 500
    Received: 1280

      339 |
      340 |         // State should now be 500
    > 341 |         expect(widthProviderWrapper.state().width).toEqual(500);
          |                                                    ^
      342 |       });
      343 |     });
      344 |

      at Object.toEqual (test/spec/lifecycle-test.js:341:52)
      ```

@currents-jantu
Copy link
Contributor Author

@currents-jantu I am now realizing this broke the resize tests. Could you please fix it?

 FAIL  test/spec/lifecycle-test.js
  ● Lifecycle tests › <ReactGridLayout> › WidthProvider › Renders with WidthProvider measureBeforeMount

    expect(received).toEqual(expected) // deep equality

    Expected: "ReactGridLayout"
    Received: "div"

      318 |
      319 |         // Should have removed the div, now has the RGL
    > 320 |         expect(wrapper.childAt(0).childAt(0).name()).toEqual("ReactGridLayout");
          |                                                      ^
      321 |         expect(wrapper.childAt(0).state().width).toEqual(500);
      322 |       });
      323 |

      at Object.toEqual (test/spec/lifecycle-test.js:320:54)

  ● Lifecycle tests › <ReactGridLayout> › WidthProvider › WidthProvider responds to window resize events

    expect(received).toEqual(expected) // deep equality

    Expected: 500
    Received: 1280

      339 |
      340 |         // State should now be 500
    > 341 |         expect(widthProviderWrapper.state().width).toEqual(500);
          |                                                    ^
      342 |       });
      343 |     });
      344 |

      at Object.toEqual (test/spec/lifecycle-test.js:341:52)
      ```

Sure, I will work on the test.

@currents-jantu
Copy link
Contributor Author

@STRML current test cases won't work with ResizeObserver. I guess resizeObserver needs to be mocked here, I can try to rewrite these tests though. What you are suggesting ?

@STRML
Copy link
Collaborator

STRML commented Sep 11, 2023

I don't know how to fix it - really a question for you. For now I will remove the test on master as not to break other builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core use this label for changes in `lib` directory deps use this label for changes in the project's dependencies infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants