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

fix(ui-common): prevent null values for string cells in HandsonTable component #1678

Closed
wants to merge 1 commit into from

Conversation

hdinia
Copy link
Member

@hdinia hdinia commented Jul 27, 2023

fix #1663

-This Pull Request addresses an issue in the TableMode, when you press the delete button, the default behavior is to set the cell value to null. This behavior is now modified so that the cell's value is set to an empty string ("") instead.

Note: only string inputs are concerned

@hdinia hdinia requested a review from skamril July 27, 2023 19:27
@commit-lint
Copy link

commit-lint bot commented Jul 27, 2023

Bug Fixes

  • ui-common: prevent null values for string cells in HandsonTable component (e9eb98c)

Contributors

skamril

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@hdinia hdinia force-pushed the feature/1663-table-mode-update-issue branch from 02728f8 to 0432b24 Compare July 31, 2023 14:37
@hdinia hdinia force-pushed the feature/1663-table-mode-update-issue branch from 0432b24 to 70684f0 Compare July 31, 2023 14:38
@@ -48,7 +48,7 @@ function Table(props: TableProps) {
const handleAfterChange: HandsontableProps["afterChange"] =
function afterChange(this: unknown, changes, ...rest): void {
changes?.forEach(([row, column, _, nextValue]) => {
setValue(`${data[row].id}.${column}`, nextValue);
setValue(`${data[row].id}.${column}`, nextValue ?? "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Like I said, nextValue is not necessary a string.

Copy link
Member Author

@hdinia hdinia Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if I am wrong but null value is possible only on strings. when you press delete on booleans true or false only is possible. on numbers inputs this behavior is disabled.

So I dont see in what case we can have an other type than string that is null ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT:

afterChange event in Handsontable provides the previous value of the cell prevValue, in addition to nextValue. We can leverage this to determine if the cell was originally a string. If it was, and if nextValue is null, we can then replace it with an empty string. This ensures that we only replace null with "" for cells that were originally strings.

  const handleAfterChange: HandsontableProps["afterChange"] =
    function afterChange(this: unknown, changes, ...rest): void {
      changes?.forEach(([row, column, prevValue, nextValue]) => {
        if (typeof prevValue === "string" && nextValue === null) {
          setValue(`${data[row].id}.${column}`, "");
        } else {
          setValue(`${data[row].id}.${column}`, nextValue);
        }
      });
      restProps.afterChange?.call(this, changes, ...rest);
    };

Tested, it works as expected, I dont see a better solution if this does not suits you I let you provide a better one

Copy link
Member

@skamril skamril Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use beforeChange instead, because is called before re-render.

const handleBeforeChange: HandsontableProps["beforeChange"] =
  function beforeChange(this: unknown, changes, ...rest): void {
    changes.forEach(([row, prop, oldValue, newValue]) => {
      if (restProps.allowEmpty && typeof oldValue === "string" && newValue === null) {
        // You must mutate the value
        changes[row][3] = '';
      }
    });
    restProps.beforeChange?.call(this, changes, ...rest);
  }

Add it to the HandsonTable component.

@skamril skamril self-requested a review August 3, 2023 15:37
@hdinia hdinia force-pushed the feature/1663-table-mode-update-issue branch from 70684f0 to 19ce885 Compare August 4, 2023 08:21
@skamril skamril force-pushed the feature/1663-table-mode-update-issue branch from 19ce885 to 96078ed Compare August 4, 2023 15:06
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Aug 4, 2023
@skamril skamril self-assigned this Aug 4, 2023
@skamril skamril removed their request for review August 4, 2023 15:07
skamril
skamril previously approved these changes Aug 4, 2023
@skamril skamril force-pushed the feature/1663-table-mode-update-issue branch from 96078ed to 07f3566 Compare August 4, 2023 15:24
@hdinia hdinia force-pushed the feature/1663-table-mode-update-issue branch from 07f3566 to 1c32d90 Compare August 7, 2023 07:50
@skamril skamril changed the title fix(ui-tablemode): replace null cell values with empty strings fix(ui-common): prevent null values for string cells in HandsonTable component Aug 7, 2023
@skamril skamril force-pushed the feature/1663-table-mode-update-issue branch from 1c32d90 to 07f3566 Compare August 7, 2023 08:33
@skamril skamril force-pushed the feature/1663-table-mode-update-issue branch from 07f3566 to e9eb98c Compare August 7, 2023 08:35
@skamril skamril assigned hdinia and unassigned hdinia Aug 7, 2023
@skamril skamril requested review from laurent-laporte-pro and skamril and removed request for laurent-laporte-pro and skamril August 7, 2023 08:47
@skamril skamril requested review from skamril and removed request for skamril August 7, 2023 08:49
@skamril skamril closed this Aug 7, 2023
@skamril skamril deleted the feature/1663-table-mode-update-issue branch August 7, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ANT-804] Saving modifications impossible on table mode
2 participants