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(editor): add default value to progress column #9637

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from

Conversation

good-jinu
Copy link

Fixes #9624

What's the issue?

2025-01-11.2.57.36.mov
  1. Add a new column with the data type "progress"
  2. Apply a filter to the "progress" column with the condition "= 0"
  3. Observe that no rows are displayed in the filtered results
  4. Modify the filter condition to "is empty"
  5. Observe that all rows are now displayed in the filtered results
  6. Change the value of each "progress" field to a non-zero value
  7. Change the value of each "progress" field back to zero
  8. Apply the filter to the "progress" column with the condition "= 0" again
  9. Observe that all rows with a "progress" value of zero are now displayed, which contradicts the initial behavior where no rows were displayed with the same filter condition

What's the root cause?

New rows are initialized to null by default, resulting in empty values.

What's changed?

2025-01-11.3.12.19.mov
  1. [In code] Added defaultValue field within the PropertyConfig type
  2. [In code] Added defaultValue with zero at progressPropertyModelConfig
  3. Automatically assigned the default value to cells when a new property is added.
  4. Updated cell values to the default value whenever a property type is modified.
  5. Set newly created rows to the default value for all properties

@good-jinu good-jinu requested a review from a team as a code owner January 10, 2025 18:14
@graphite-app graphite-app bot requested a review from forehalo January 10, 2025 18:14
Copy link

graphite-app bot commented Jan 10, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

);

updateCells(
Copy link
Member

Choose a reason for hiding this comment

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

This will cause a large number of update records to exist in the CRDT. It is recommended not to do this. Instead, a default value should be returned when obtaining the value.

Copy link
Author

Choose a reason for hiding this comment

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

This will cause a large number of update records to exist in the CRDT. It is recommended not to do this. Instead, a default value should be returned when obtaining the value.

Thank you for super fast comment! @zzj3720

I'm not sure of how to implement returning a default value when obtaining the value. Could you please provide more guidance or an example to assist me in resolving this issue? I would greatly appreciate it.

Copy link
Author

Choose a reason for hiding this comment

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

/blocksuite/affine/data-view/src/property-presets/progress/define.ts
@@ -8,16 +8,16 @@ export const progressPropertyModelConfig =
     name: 'Progress',
     type: () => t.number.instance(),
     defaultData: () => ({}),
-    cellToString: ({ value }) => value?.toString() ?? '',
+    cellToString: ({ value }) => value?.toString() ?? '0',
     cellFromString: ({ value }) => {
-      const num = value ? Number(value) : NaN;
+      const num = value ? Number(value) : 0;
       return {
-        value: isNaN(num) ? null : num,
+        value: isNaN(num) ? 0 : num,
       };
     },
-    cellToJson: ({ value }) => value ?? null,
+    cellToJson: ({ value }) => value ?? 0,
     cellFromJson: ({ value }) => {
-      if (typeof value !== 'number') return undefined;
+      if (typeof value !== 'number') return 0;
       return value;
     },
     isEmpty: () => false,

How about changing all null or undefined values to return 0 like this? Although this change will internally store as null, it will work correctly when filtering. @zzj3720

Copy link
Author

@good-jinu good-jinu left a comment

Choose a reason for hiding this comment

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

Leave any comment please! 🙏

All feedback, positive or negative, is welcome!


const metaConfig = this.propertyMetaGet(
type ?? propertyPresets.multiSelectPropertyConfig.type
);
Copy link
Author

Choose a reason for hiding this comment

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

propertyMetaGet method looks like this

  propertyMetaGet(type: string): PropertyMetaConfig {
    return databaseBlockAllPropertyMap[type];
  }

It returns the property configuration object corresponding the type.

Object.fromEntries(
this.rows$.value.map(r => [r, metaConfig.config.defaultValue])
)
);
Copy link
Author

Choose a reason for hiding this comment

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

it updates cells to default value whenever the property is added

@@ -21,4 +21,5 @@ export const progressPropertyModelConfig =
return value;
},
isEmpty: () => false,
defaultValue: 0,
Copy link
Author

Choose a reason for hiding this comment

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

The progress value always ranges from 0 to 100. When a new progress item is created, it is initially set to 0.

const cells: Record<string, unknown> = {};
currentCells.forEach((value, i) => {
if (value == null && result.cells[i] == null) {
cells[rows[i]] = metaConfig.config.defaultValue;
}
Copy link
Author

Choose a reason for hiding this comment

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

it updates cells to default value whenever the property is updated to other type.

column.id,
this.propertyMetaGet(column.type$.value).config.defaultValue
)
);
Copy link
Author

Choose a reason for hiding this comment

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

It initialize the cells to default value when a row is added

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.

​Progress in Database initially to be a Empty rather than Zero
2 participants