Adds editing options for progress and rating columns in react demo. #9935

bulaj · 2022-09-21T10:56:40Z

Context

Adds editing options for progress and rating columns in react demo.

How has this been tested?

Manually in browser.

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):

Progress and Rating columns should be editable in the main demo #9581

Affected project(s):

Checklist:

I have reviewed the guidelines about Contributing to Handsontable and I confirm that my code follows the code style of this project.
I have signed the Contributor License Agreement
My change requires a change to the documentation.

[skip changelog]

To prevent errors with development server.

github-actions · 2022-09-21T11:01:08Z

Launch the local version of documentation by running:

npm run docs:review fd074cddfcb0b92de89da80948dc700fd758b490

…dsontable into feature/issue-9581-demo

warpech · 2022-09-23T12:37:13Z

@krzysztofspilka to run the project, run the following commands in the repo root:

git fetch
git checkout feature/issue-9581-demo
git pull
cd examples/12.1.2/docs/react/demo
npm i
npm run start

And open http://localhost:8080/ in your web browser

krzysztofspilka · 2022-09-26T13:25:02Z

@warpech thanks for help, I'm on it.

krzysztofspilka · 2022-09-26T14:40:29Z

@bulaj I can't tell much about the quality of the code, but from the UX/UI standpoint, there are some issues that we should fix before rolling this out. See the list below.

An empty cell in the column "Progress" is represented by a 100% stacked bar chart. Test: click on any cell in the column "Progress" and then press DELETE. Alternatively, open the column menu and choose the option "Clear column".
All keyboard shortcuts from this list: https://handsontable.com/docs/keyboard-shortcuts/#edition-keyboard-shortcuts should be available for both the "Progress" and "Rating" columns.
I can paste non-numeric values to both the "Progress" and "Rating" columns, and apparently the renderer will try to handle them. Try pasting 12394-3841-34. When you open the editor of such a cell it is empty.

bulaj · 2022-09-28T09:59:30Z

@krzysztofspilka 1 and 3 should be fixed now.

warpech

Partial review. @jansiegel we will need to finish this.

warpech · 2022-09-23T10:00:09Z

examples/12.1.2/docs/react/demo/src/constants.ts

-  ]
-];
+export const data = [{
+  0: false,


This entry is super confusing, I think we need to refactor it or remove it (is it even used)?

warpech · 2022-09-30T13:37:57Z

examples/12.1.2/docs/react/demo/src/index.tsx

@@ -46,24 +47,25 @@ const App = () => {
      beforeRenderer={addClassesToRows}
      afterGetRowHeader={drawCheckboxInRowHeaders}
      afterOnCellMouseDown={changeCheckboxCell}
+      beforePaste={omitNonNumericPaste}


This is to achieve the following requirement:

I can paste non-numeric values to both the "Progress" and "Rating" columns, and apparently the renderer will try to handle them. Try pasting 12394-3841-34. When you open the editor of such a cell it is empty.

I think that this implementation is not as good as it could be. The best way to disallow certain inputs is to use a validator function and allowInvalid: false configuration option. This is how e.g. date picker does it.

…eature/issue-9581-demo

* improve React demo * improve React demo

jansiegel

@maciejbo-handsontable
I'm not sure what is the exact scope of this PR. Could you list the requirements that are supposed to be met here?
The PR description mentions only making the progress and rating columns editable, but the PR you previously used to make changes was described vaguely as improve React demo.

That being said, I've looked at the demo after the modifications introduced by the PR and found these:

This PR modifies the data used in the example. After the changes, the dataset consists of list of 5 items duplicated five times. I don't think demos in other frameworks do that. Was this change intentional?
The Progress column allows entering non-numeric data, which results in the progress bar being displayed as 100%. Works the same with pasted data.
The Rating column doesn't accept non-numeric data when editing but does accept pasted non-numeric data, which results in the cell being cleared.
The demo contains some logic which I'm assuming was meant to horizontally align the header contents, but it targets only columns 9, 10 and 12 and there are no such columns in this example.
The state of the row header checkboxes is stored at (row, "0") coordinates, which I find a bit weird.

If any of them is outside this task's scope, let me know.

maciejbo-handsontable · 2023-02-04T01:23:47Z

There are still some unanswered questions, @adamu-handsontable, could you clarify the scope of this task?

Hey, what I can say is I was asked to "finish this PR which title says >editing options for prgress and rating<" and with that being said I was trying not to touch what was not necessary.

1. This PR modifies the data used in the example. After the changes, the dataset consists of 5 items duplicated five times. This changes the way the table looks, and if it was ever synchronized to the `visual-tests` demo sets (which I'm guessing it will at some point?) will break the test scenarios. [[original comment](https://github.com/handsontable/handsontable/pull/9935#pullrequestreview-1277235908)]

With first paragraph being said - normally I wouldn't touch this thing, but I needed to implement some related stuff, so I changed this entire file cause in my opinion it would be easier to change in future 5 places instead of 50 - but now, cause of your next points, I brought previous version back.

2. The demo contains some logic which I'm assuming was meant to align the header contents horizontally, but it targets only columns 9, 10, and 12 - and there are no such columns in this example. [[original comment](https://github.com/handsontable/handsontable/pull/9935#pullrequestreview-1277235908)]

If I understand your message correctly, the same thing exists in rest of demos also and was brought here by guy who started this thing before me - I wouldn't know what was the reason, maybe some hidden functionality. Without this part what I see still works, but need confirmation if I have to do something with that in this PR.

3. The state of the row header checkboxes is stored at (row, "0") coordinates, which I find a bit weird. [[original comment](https://github.com/handsontable/handsontable/pull/9935#pullrequestreview-1277235908)]

As above - If I understand your message correctly, the same thing exists in rest of demos also. Need confirmation if I have to do something with that in this PR.

4. When comparing the demo from this PR and the demo from `develop`, they're different not only because of the modified dataset, but also:
   
   * The `develop` demo has some rows pre-selected
   * The `develop` demo has some checkboxes from the column `In stock` unchecked by default
   
   Should they be unified?

Hey, that's true - I think they should be and they are unified already.

5. We could work on the progress bar and star renderers/editors further, for example, limit the input to a specific range etc (currently, entering a value of 10000 will successfully save it to the edited cell), but, again, I don't know how much detailed are they supposed to be.

this need to be confirmed by @adamu-handsontable , but what I can say is that @krzysztofspilka tested this thing before pushing here and didn't raise this point.

adamu-handsontable · 2023-02-07T21:11:51Z

As a result of this task I'd like to have a ready-to-use example with editable Progress and Rating. I see that there are some changes which are not required to achieve it. The original PR was done some time ago and got probably outdated. Our basis is the current example.

Keep the array of object type structure, but make data equivalent to current example from develop. Change order of columns to match the current example from develop.
If there no such columns, then it seems not necessary, please check if it is not required and if so, remove it.
Yes this is strange, please change it to something more meaningful like: isChecked and adjust drawCheckboxInRowHeaders and changeCheckboxCell and any other function that uses it.
Yes, they should be unified and adjusted to match current develop
Progress bar should accept the range of Floats from 0 to 100. Ratings should accept Integers from 1 to 5.

jansiegel

I think all my previous points have been addressed, but one of @adamu-handsontable's points is missing:

Progress bar should accept the range of Floats from 0 to 100. Ratings should accept Integers from 1 to 5.

So I'm approving this PR and passing the review of that one point to @adamu-handsontable .

jansiegel · 2023-02-16T09:21:49Z

examples/next/docs/react/demo/src/renderers/ProgressBar.tsx

+export const ProgressBarRenderer = (props: HandsontableProps) => {
+  if (props.cellProperties.valid !== false) {


@maciejbo-handsontable
I found one additional issue with the demo - when you pass invalid data for the progress bar or stars columns, they're not marked as invalid.
That happens because Handsontable doesn't trigger validation by default when loading data.
To get around it it, you can manually trigger the validator if it wasn't triggered already.

Take a look at my suggestion, and if you think it works all right, could you implement it for the stars renderer as well?

Suggested change

export const ProgressBarRenderer = (props: HandsontableProps) => {

if (props.cellProperties.valid !== false) {

export const ProgressBarRenderer = (props: HandsontableProps) => {

let isValid = props.cellProperties.valid;

// Run the validator for the cell at initialization.

if (isValid === void 0) {

(props.cellProperties.validator as Function)(props.value, (isValueValid: boolean) => {

isValid = isValueValid;

});

}

if (isValid === true) {

adamu-handsontable · 2023-02-16T12:15:47Z

Let's skip the validation of stars and progress for now. We'll do that in a separate PR later if necessary.

…baseRenderer calls in the custom renderers.

…dsontable into feature/issue-9581-demo

wszymanski

I have some suggestions. I'll test example after changes in code.

wszymanski · 2023-03-21T10:24:38Z

examples/next/docs/react/demo/src/hooksCallbacks.ts

-  if (cellProperties.instance.getDataAtRowProp(row, "0")) {
+  const rowData = cellProperties.instance.getSourceDataAtRow(row) as RowObject;
+
+  if (!!rowData?.checked) {


Suggested change

if (!!rowData?.checked) {

if (rowData?.checked !== true) {

wszymanski · 2023-03-21T10:26:12Z

examples/next/docs/react/demo/src/hooksCallbacks.ts

+  const rowData = this.getSourceDataAtRow(row) as RowObject;
+
+  input.type = 'checkbox';
+  input.checked = !!rowData?.checked;


Suggested change

input.checked = !!rowData?.checked;

input.checked = rowData?.checked === true;

wszymanski · 2023-03-21T10:34:27Z

examples/next/docs/react/demo/src/constants.ts

+export const MESSAGE = {
+  BAD_VALUE: '#bad-value#'
+};


Do we need to introduce this variable using object?

Suggested change

export const MESSAGE = {

BAD_VALUE: '#bad-value#'

};

export const BAD_VALUE_MESSAGE = '#bad-value#';

wszymanski · 2023-03-21T10:37:52Z

examples/next/docs/react/demo/src/renderers/Stars.tsx

+  // Run the validator for the cell at initialization.
+  if (isValid === void 0) {
+    (props.cellProperties.validator as Function)(props.value, (isValueValid: boolean) => {
+      isValid = isValueValid;
+    });
+  }


The same as in ProgressBar.tsx.

wszymanski · 2023-03-21T10:40:57Z

examples/next/docs/react/demo/src/renderers/ProgressBar.tsx

+  // Run the validator for the cell at initialization.
+  if (isValid === void 0) {
+    (cellProperties.validator as Function)(value, (isValueValid: boolean) => {
+      isValid = isValueValid;
+    });
+  }


Maybe its worth to move up method, beyond the renderer declaration, to not repeat function creation and make it more clear by naming it. Suggestion up to you.

krzysztofspilka · 2023-09-01T11:51:24Z

@wszymanski as discussed, you will be assigned to this task with someone else from the Dev team as reviewer. Please accept all your remarks to the code. Let's treat this as a work in progress, thanks.

Maciej Bulczak and others added 4 commits September 8, 2022 19:39

Update react-scripts package (#9581)

c9b701b

To prevent errors with development server.

Change data structure (#9581)

1d67c9b

Add editing for progress and rating columns in react demo (#9581)

ce66e96

Merge branch 'develop' into feature/issue-9581-demo

d4e1c6f

bulaj added the Docs: Tools Issues related to Handsontable's documentation tools (VuePress, deployment, scripts etc.) label Sep 21, 2022

bulaj requested a review from krzysztofspilka September 21, 2022 11:04

Maciej Bulczak added 2 commits September 22, 2022 11:16

Update docs to reflect test flow (#9581)

f4490ff

Merge branch 'feature/issue-9581-demo' of github.com:handsontable/han…

1efcb81

…dsontable into feature/issue-9581-demo

bulaj changed the title ~~Feature/issue 9581 demo~~ Adds editing options for progress and rating columns in react demo. Sep 22, 2022

Maciej Bulczak added 2 commits September 28, 2022 10:44

Fix display of empty values in progress bar (#9581)

29c3849

Block pasting non-numeric values in progress and rating columns (#9581)

b7ea8c3

Block pasting non-numeric values in progress and rating columns (#9581)

1bb7697

warpech requested changes Sep 30, 2022

View reviewed changes

krzysztofspilka linked an issue Oct 18, 2022 that may be closed by this pull request

Progress and Rating columns should be editable in the main demo #9581

Open

krzysztofspilka removed their request for review October 24, 2022 19:20

maciejbo-handsontable and others added 7 commits January 18, 2023 10:36

Merge branch 'develop' of github.com:handsontable/handsontable into f…

35c7b34

…eature/issue-9581-demo

demo improvements

9dc48b6

improve React demo (#10218)

ff5d204

* improve React demo * improve React demo

improve React demo

162c5e6

improve React demo

448aebd

improve React demo

ecc52bb

improve React demo

7b35216

maciejbo-handsontable requested a review from jansiegel January 25, 2023 10:12

maciejbo-handsontable self-assigned this Jan 25, 2023

jansiegel requested changes Jan 31, 2023

View reviewed changes

React demo improvements

3267f05

React demo improvements

c33cd2f

maciejbo-handsontable added 5 commits February 9, 2023 15:16

React demo improvements

d4e65a8

imrpove React demo

64c6b42

improve checkboxes rendering in React demo

914aae0

improve checkboxes rendering in React demo

66f842a

improve checkboxes rendering in React demo

2a50c48

jansiegel approved these changes Feb 14, 2023

View reviewed changes

jansiegel requested a review from adamu-handsontable February 14, 2023 12:55

jansiegel requested changes Feb 16, 2023

View reviewed changes

force cell data validation on init, not only on edit

76d314b

jansiegel mentioned this pull request Feb 16, 2023

Add Vue 3 demo to examples #10245

Open

12 tasks

fix typo in comment

0161d06

jansiegel previously approved these changes Feb 16, 2023

View reviewed changes

jansiegel assigned jansiegel and unassigned maciejbo-handsontable Feb 17, 2023

jansiegel and others added 3 commits March 10, 2023 10:31

Merge branch 'develop' into feature/issue-9581-demo

fd074cd

Remove the progress bar positioning workaround in favor of using the …

754e7c5

…baseRenderer calls in the custom renderers.

Merge branch 'feature/issue-9581-demo' of github.com:handsontable/han…

b393eec

…dsontable into feature/issue-9581-demo

jansiegel dismissed their stale review via b393eec March 10, 2023 10:22

jansiegel requested a review from wszymanski March 14, 2023 10:16

wszymanski reviewed Mar 21, 2023

View reviewed changes

krzysztofspilka mentioned this pull request Jun 7, 2023

Progress and Rating columns should be editable in the main demo #9581

Open

krzysztofspilka removed the request for review from adamu-handsontable September 1, 2023 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds editing options for progress and rating columns in react demo. #9935

Adds editing options for progress and rating columns in react demo. #9935

bulaj commented Sep 21, 2022 •

edited by maciejbo-handsontable

github-actions bot commented Sep 21, 2022 •

edited

warpech commented Sep 23, 2022 •

edited

krzysztofspilka commented Sep 26, 2022

krzysztofspilka commented Sep 26, 2022

bulaj commented Sep 28, 2022

warpech left a comment •

edited

warpech Sep 23, 2022

warpech Sep 30, 2022

jansiegel left a comment

maciejbo-handsontable commented Feb 4, 2023 •

edited

adamu-handsontable commented Feb 7, 2023

jansiegel left a comment

jansiegel Feb 16, 2023 •

edited

adamu-handsontable commented Feb 16, 2023

wszymanski left a comment

wszymanski Mar 21, 2023

wszymanski Mar 21, 2023

wszymanski Mar 21, 2023

wszymanski Mar 21, 2023

wszymanski Mar 21, 2023

krzysztofspilka commented Sep 1, 2023

		export const ProgressBarRenderer = (props: HandsontableProps) => {
		if (props.cellProperties.valid !== false) {

	input.checked = !!rowData?.checked;
	input.checked = rowData?.checked === true;

Adds editing options for progress and rating columns in react demo. #9935

Are you sure you want to change the base?

Adds editing options for progress and rating columns in react demo. #9935

Conversation

bulaj commented Sep 21, 2022 • edited by maciejbo-handsontable

Context

How has this been tested?

Types of changes

Related issue(s):

Affected project(s):

Checklist:

github-actions bot commented Sep 21, 2022 • edited

warpech commented Sep 23, 2022 • edited

krzysztofspilka commented Sep 26, 2022

krzysztofspilka commented Sep 26, 2022

bulaj commented Sep 28, 2022

warpech left a comment • edited

Choose a reason for hiding this comment

warpech Sep 23, 2022

Choose a reason for hiding this comment

warpech Sep 30, 2022

Choose a reason for hiding this comment

jansiegel left a comment

Choose a reason for hiding this comment

maciejbo-handsontable commented Feb 4, 2023 • edited

adamu-handsontable commented Feb 7, 2023

jansiegel left a comment

Choose a reason for hiding this comment

jansiegel Feb 16, 2023 • edited

Choose a reason for hiding this comment

adamu-handsontable commented Feb 16, 2023

wszymanski left a comment

Choose a reason for hiding this comment

wszymanski Mar 21, 2023

Choose a reason for hiding this comment

wszymanski Mar 21, 2023

Choose a reason for hiding this comment

wszymanski Mar 21, 2023

Choose a reason for hiding this comment

wszymanski Mar 21, 2023

Choose a reason for hiding this comment

wszymanski Mar 21, 2023

Choose a reason for hiding this comment

krzysztofspilka commented Sep 1, 2023

bulaj commented Sep 21, 2022 •

edited by maciejbo-handsontable

github-actions bot commented Sep 21, 2022 •

edited

warpech commented Sep 23, 2022 •

edited

warpech left a comment •

edited

maciejbo-handsontable commented Feb 4, 2023 •

edited

jansiegel Feb 16, 2023 •

edited