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

Improving React Developer Experience #10831

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open

Conversation

NOtherDev
Copy link

Context

Implementing all the points as discussed in RFC #10767.

How has this been tested?

  • new features covered with passing unit tests
  • all existing unit tests still passing
  • all the demo projects work correctly

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. RFC [RFC] Improving React Developer Experience #10767
  2. Each of the commits was covered by a separately reviewed PR: https://github.com/handsontable/handsontable/pulls?q=is%3Amerged+is%3Apr+author%3A%40me+

Affected project(s):

  • handsontable
  • @handsontable/angular
  • @handsontable/react
  • @handsontable/vue
  • @handsontable/vue3

Checklist:

NOtherDev and others added 9 commits February 19, 2024 09:37
* 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
…nd HotTable to use render props approach (#10791)

* Change the way renderer might be passed to HotColumn and HotTable to use render props approach (#4)

* redux tests

* changelog

* build fix
… HotTable to use render props approach (#10793)

* Change the way editor might be passed to HotColumn and HotTable to use render props approach (#5)

* 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
@NOtherDev NOtherDev self-assigned this Feb 28, 2024
* Fix for react-redux.md docs compilation

* Fix for react-redux.md docs - split renderer from editor
Copy link

github-actions bot commented Feb 28, 2024

Launch the local version of documentation by running:

npm run docs:review 65653097635a5506f23860d6f9a4ac2c08ca5cf1

@NOtherDev NOtherDev marked this pull request as ready for review February 28, 2024 21:49
@evanSe
Copy link
Member

evanSe commented Mar 7, 2024

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 0.0.0-next-d093c11-20240307)

image

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
https://www.npmjs.com/package/@handsontable/react/v/0.0.0-next-d093c11-20240307?activeTab=code

@evanSe
Copy link
Member

evanSe commented Mar 7, 2024

@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

  • super being called behind the scenes in the useHotEditor we should be able to hide this from the user
  • the current setValue and getValue is not exposed to the user as they are conceptually different from react
  • pass a configuration object to the useHotEditor rather than a function

potentially how useHotEditor could look
image

rest of the example

image

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
@NOtherDev
Copy link
Author

@evanSe Here's the PR to sync with the new guides structure - basically merge with develop: #10848

* 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 <[email protected]>
Co-authored-by: Evan Seaward <[email protected]>
@evanSe
Copy link
Member

evanSe commented Mar 11, 2024

@evanSe Here's the PR to sync with the new guides structure - basically merge with develop: #10848

did not seem to work, might be worth a rebase

@NOtherDev
Copy link
Author

Yeah, sorry @evanSe, fixed. Forgot I can do it directly here 🙂

@NOtherDev
Copy link
Author

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 0.0.0-next-d093c11-20240307)

image 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 https://www.npmjs.com/package/@handsontable/react/v/0.0.0-next-d093c11-20240307?activeTab=code

@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 @handsontable/react@latest, which does not resolve to 0.0.0-next-d093c11-20240307 (as this is next, not latest version), so the demo project does not compile with the not-updated React wrapper. Should I fix the latest reference to next? Or how should this issue be handled?

@evanSe
Copy link
Member

evanSe commented Mar 12, 2024

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 0.0.0-next-d093c11-20240307)
image
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 https://www.npmjs.com/package/@handsontable/react/v/0.0.0-next-d093c11-20240307?activeTab=code

@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 @handsontable/react@latest, which does not resolve to 0.0.0-next-d093c11-20240307 (as this is next, not latest version), so the demo project does not compile with the not-updated React wrapper. Should I fix the latest reference to next? Or how should this issue be handled?

with previous versions of the wrapper the type definitions were published in the root directory and now instead they are in src/, something is probably broken in a build process so no need to worry about it

@NOtherDev
Copy link
Author

Hey guys @evanSe @budnix @jansiegel - is there anything I can do to help moving this PR forward?
The longer it waits the more conflicts will there be to resolve - I've already resolved docs conflicts once and there's more now.

@evanSe
Copy link
Member

evanSe commented Mar 27, 2024

Hey guys @evanSe @budnix @jansiegel - is there anything I can do to help moving this PR forward? The longer it waits the more conflicts will there be to resolve - I've already resolved docs conflicts once and there's more now.

@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 😃

Copy link
Member

@jansiegel jansiegel left a 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:

  1. 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
  1. 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.

@NOtherDev
Copy link
Author

  1. 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

@evanSe You mentioned it is related with the build process change of the TS declarations, am I right?

  1. 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.

Native ones are still available via hotRerender and hotEditor respectively.

@jansiegel
Copy link
Member

Native ones are still available via hotRerender and hotEditor respectively.

Oh, alright, I missed that 👍

Copy link
Member

@budnix budnix left a 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.

@evanSe
Copy link
Member

evanSe commented Apr 22, 2024

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

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.

None yet

4 participants