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

**Breaking:** Fix operational context #921

Merged
merged 12 commits into from
Feb 15, 2019
Merged

Conversation

fabien0102
Copy link
Contributor

Summary

image

  • Make operational context component class compliante
  • Remove windowSize on context (huge performance leak)
  • Add some todo for the future (we need to refactor this tooltip component)

Related issue

To be tested

Me

  • No error or warning in the console on localhost:6060

Tester 1

  • Things look good on the demo.

Tester 2

  • Things look good on the demo.

@@ -0,0 +1,23 @@
import { useEffect, useState } from "react"

const useWindowSize = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have readme and tests for this hook

Copy link
Contributor

Choose a reason for hiding this comment

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

Add them.


export { Context, WindowSize, IMessage, MessageType, useOperationalContext }
export default Consumer
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should still export the types?

@@ -0,0 +1,23 @@
import { useEffect, useState } from "react"

const useWindowSize = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add them.

src/hooks/useWindowSize/useWindowSize.ts Outdated Show resolved Hide resolved
@TejasQ TejasQ changed the title **Breaking:**: Fix operational context **Breaking:** Fix operational context Feb 15, 2019
@TejasQ
Copy link
Contributor

TejasQ commented Feb 15, 2019

image

Do either of you test locally?

Copy link
Contributor

@TejasQ TejasQ left a comment

Choose a reason for hiding this comment

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

LOOKS GREAT

@TejasQ TejasQ merged commit d9ab7e5 into master Feb 15, 2019
@TejasQ TejasQ deleted the fix-operational-context branch February 15, 2019 09:54
@TejasQ TejasQ mentioned this pull request Feb 15, 2019
19 tasks
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.

4 participants