-
-
Notifications
You must be signed in to change notification settings - Fork 241
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
Grid.js v6 - Functional Components #1267
Conversation
Size Change: -25.8 kB (-26%) 🎉 Total Size: 72 kB
|
jest coverage report 🧪❌ The test suite failed. Please, check the console output for more details. |
Geez @afshinm you have been busy during the holidays! |
h, | ||
createElement, | ||
Component, | ||
createRef, | ||
useEffect, | ||
useRef | ||
useRef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we re-export preact's exports? I'm curious!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's mainly for people who want to extend Grid.js or write Grid.js components. Note that when we compile Grid.js, Preact is already included in the output (i.e you don't have to install Preact separately).
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success176 tests passing in 31 suites. Report generated by 🧪jest coverage report action from 95d8564 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the limited time I had to review this, there is too much to absorb, but thank you for the work. Great job.
It should make contributing much easier in the future.
💪 👏 🍺
document.removeEventListener('touchend', end); | ||
}; | ||
|
||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot going on here so I haven't fully absorbed the whole change, but it might be worth considering wrapping the mouse event in useCallback
to memoize them? I had performance issues in the past using mouse listeners.
It might be unnecessary here, I don't have a clear picture of the code logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
@@ -1,16 +1,18 @@ | |||
import { h } from 'preact'; | |||
import { h, JSX } from 'preact'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes might fix #1081
src/view/table/th.tsx
Outdated
const [style, setStyle] = useState({}); | ||
const { dispatch } = useStore(); | ||
|
||
useEffect(() => { | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we wrap this in a 0 setTimeout? Is it a racing condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover probably, removed it
expect(mock).toBeCalledWith('123'); | ||
return new Promise<void>((resolve) => { | ||
// TODO: can we fix this and remove the setTimeout? | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know with Testing library you have access to a 'waitfor' function that basically tries your assertion for X seconds.
Maybe enzyme has something similar?
Or we could rig our own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning on migrating to the React Testing Library which could potentially help us remove these setTimeouts.
"declaration": true, | ||
"baseUrl": "./", | ||
"paths": { | ||
"react": ["./node_modules/preact/compat/"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's interesting, what's the logic behind this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely remember that was a short-term solution for a weird Preact/TypeScript bug. We should be able to remove this in the future.
Co-authored-by: abitbetterthanyesterday <[email protected]>
@abitbetterthanyesterday thanks for your feedback. I have made some changes to this PR based on your feedback. Let me know if we're good to merge. |
@daniel-werner @salamaashoush That would be great if you guys can also review this PR and give me feedback ❤️ |
@afshinm I took a quick look, and looks good at first sight, however I am not familiar with the code, and the PR is quite big. I'll let you know, and contribute if I have some problems integrating/using it in my laravel datagrid package. |
Grid.js v6 🚀
Context
This PR updates Grid.js and most internal components to work with Functional Preact components instead of class components. In addition to that, I have worked on simplifying the
Config
object and the state management process.Notable changes are:
useConfig
,useContext
anduseStore
)Store
objectWhy?
Simplified Config and State management process along with Functional components (instead of class components) makes Grid.js much easier to maintain and manage. It also reduces the build output size by about 30% which is a great win.
Feedback
I understand this is a huge PR (15k lines changed) and I apologise. It's almost impossible to break down this PR into smaller change sets but I'm happy to provide more context if anyone has any questions.