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
Add Vue 3 demo to examples #10245
base: develop
Are you sure you want to change the base?
Add Vue 3 demo to examples #10245
Conversation
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.
Apart from my suggestions in the code, the demo needs a little correction in the styling of the progress bar.
When comparing this demo with the vue2 demo, the progress bars appear in a slightly different position, which would cause issues with the visual tests.
Take a look at this video - in here I'm switching between tabs with the vue2
and vue3
demos:
progress-bar-position.mov
@@ -0,0 +1,101 @@ | |||
<script src="./DataGrid.js"></script> |
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 think we could move the entire logic into this (DataGrid.vue
) file to keep it consistent with the vue2 demo.
@@ -0,0 +1,10 @@ | |||
import Handsontable from 'handsontable'; | |||
import { HotTable } from "@handsontable/vue3"; |
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.
In some places, there are single quites ('
) being used. In others - the double quotes ("
).
Could you unify that? Most probably to the single quotes, as that's what we use in the Handsontable codebase. (with the exception of DOM-related stuff, like declaring props in the templates)
return; | ||
} | ||
|
||
const rowData = cellProperties.instance.getSourceDataAtRow(row) as { checked: boolean }; |
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.
You can use the RowObject
type in here (and a couple of lines below).
It's available for importing from:
import type { RowObject } from 'handsontable/types/common';
const rowData = cellProperties.instance.getSourceDataAtRow(row) as { checked: boolean }; | |
const rowData: RowObject = cellProperties.instance.getSourceDataAtRow(row); |
value = 0; | ||
} | ||
|
||
const isValid = /^\d+$/.test(value.toString()); |
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.
Instead of creating a second-ish validator, you can use the native Handsontable validator.
To do that, you'd need to set the type of the column (in this case type="numeric"
) and implement logic similar to the one I suggested in your PR updating the React demo:
#9935 (comment)
value = 0; | ||
} | ||
|
||
const isValid = /^\d+$/.test(value.toString()); |
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.
Same comment as with the Progress Bar renderer.
Launch the local version of documentation by running: npm run docs:review c2deac9a4affd9d2b01ef1f508b6c84a4733be40 |
…existance of vue3 - Correct the component script type definition
@wszymanski I think the demo works correctly, and the only thing left to do is fixing the problem with the build failing on the CI. |
Co-Authored-By: Jan Siegel <[email protected]>
…ckage-lock.json file
…sontable into feature/issue-dev-1072
It seems that problem found by @jansiegel was caused by adding Investigation has shown that there are no significant differences in a way how scripts are run locally and by GA. However, I've seen some parts of code which may be problematic. Thus, I suggest to leave also another changes, not related to the problem: I also added Smoke test for Vue3 wrapper, just by copying them from Vue package. Some unnecessary changes has been reverted within f4bbebe. It seems that everything works properly, thus I'm leaving PR for review again. |
…eature/issue-dev-1072 # Conflicts: # examples/package.json
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.
@kWeb24 Could you check whether sass
isn't needed?
@kWeb24 Thank you for your effort. Perhaps I wasn't precise enough. I meant adding dev dependency 🙂 |
@wszymanski I think new dependency is not needed for just a few nested selectors. 😄 |
You are right! :) Good job. |
Context
We do not have Vue 3 demo at all, we needed to fix this lack.
How has this been tested?
Launched in browser in local environment
Types of changes
Related issue(s):
Affected project(s):
handsontable
@handsontable/angular
@handsontable/react
@handsontable/vue
@handsontable/vue3
Checklist:
[skip changelog]