-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Improving React Developer Experience #10831
base: develop
Are you sure you want to change the base?
Conversation
* Drop settings prop from HotTable * changelog file
…otTable to its children (#10789) * Wrap internal utilities into a context provided from HotTable to its children * changelog
#10799) * React wrapper: Rewrite HotTable & HotColumn into functional components * rename * renderersPortalManager * get rid of classes from tests * dont expose hotTableInner publicly * changelog
… of HotTable (#10805) * Add a check that only HotColumn instances are children of HotTable * changelog * fix circular dep
…yping everywhere (#10813) * Enable TypeScript strict mode & ensure proper strong typing everywhere; get rid of any typing. * more ts * more * last * rollback * changelog * revert
* Fix for react-redux.md docs compilation * Fix for react-redux.md docs - split renderer from editor
Launch the local version of documentation by running: npm run docs:review c4479f3578c55aa3c250ba5f8dd5dac4f9f30da5 |
One thing to note when importing the packaged version of this is that the types are in different places so we get such an error (you can try it with usually, the d.ts files are in the root https://www.npmjs.com/package/@handsontable/react?activeTab=code but in the case of the special pre-publish package I prepared they are here |
@NOtherDev so looking at whats available in the updated documentation http://localhost:8080/docs/react-data-grid/cell-editor/ the following is what I would like to see
potentially how useHotEditor could look rest of the example Maybe we can also add an improved example to the list of things to do 😄 as the old one is not very nice. |
* React wrapper: Simplify useHotEditor API * docs update * format * fix * ts fix * docs fix * docs fix * state still missing * works * destructure * typos
* Added beforeBeginEditing hook to the Cell editor guide API list (#10844) * Move code samples to separate files and update links in documentation (#10837) * include code snippets * Move code samples to separate files and update links in documentation * update demo file * Revert "Move code samples to separate files and update links in documentation" This reverts commit 1ea5f1d. * Move code samples to separate files and update links in documentation * fixup typo * fix typo * Update ESLint configuration and auto fix lint issues * fix lint * update angular demo * revert replaced dist files --------- Co-authored-by: Evan Seaward <[email protected]> * React wrapper: Sync updated docs with new structure --------- Co-authored-by: Aleksandra Budnik <[email protected]> Co-authored-by: adrianspdev <155736789 [email protected]> Co-authored-by: Evan Seaward <[email protected]>
Yeah, sorry @evanSe, fixed. Forgot I can do it directly here 🙂 |
@evanSe Actually, I think I'm not following fully. Is this about the failing Visual tests? I can see that the test tries to use |
with previous versions of the wrapper the type definitions were published in the root directory and now instead they are in |
Hey guys @evanSe @budnix @jansiegel - is there anything I can do to help moving this PR forward? |
@jansiegel has already had a review and is generally happy with the state of the pull request. Currently, we cannot merge it as we are planning on doing another release soon (14.3 on April 16th). This change is considered breaking, so we can only merge it with develop after that date. No need to worry about any merge conflicts, I will manually update them 😃 |
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 have two main notes:
- There's a problem with the types. I'm getting this error when importing
@handsontable/react
:
Could not find a declaration file for module '@handsontable/react'. '/node_modules/@handsontable/react/es/react-handsontable.mjs' implicitly has an 'any' type.
Try `npm i --save-dev @types/handsontable__react` if it exists or add a new declaration (.d.ts) file c
- Declaring the "new" renderers and editors under the "editor/renderer" props blocks the user from utilizing the vanilla-based renderers and editors (they can currently be provided that way). Not sure if it's a big issue, though.
@evanSe You mentioned it is related with the build process change of the TS declarations, am I right?
Native ones are still available via |
Oh, alright, I missed that 👍 |
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.
Everything looks good except the types. As @jansiegel said, there are some errors in TS. The types": "./index.d.ts"
entry (and exports
field) in the package.json
points to the index.d.ts
file in the root of the package, but they are missing.
Cheers, will fix this tomorrow |
@evanSe Is there anything I can help to push it forward? |
@NOtherDev Sorry did not see your message! Not at the moment, I was cleaning up the pull request and it should get released in the next version of Handsontable (15.0) |
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.
Looks good 👍
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c4479f3:
|
Context
Implementing all the points as discussed in RFC #10767.
How has this been tested?
Types of changes
Related issue(s):
Affected project(s):
handsontable
@handsontable/angular
@handsontable/react
@handsontable/vue
@handsontable/vue3
Checklist: