Table Personalization ESLint Review by lfindlaysap · Pull Request #4431 · SAP/open-ux-tools

lfindlaysap · 2026-03-18T10:42:41Z

Reviewed the ESLint texts for the table personalization rule.

changeset-bot · 2026-03-18T10:42:45Z

🦋 Changeset detected

Latest commit: 08b6bec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sap-ux/eslint-plugin-fiori-tools	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

AlinaGovoruhina

Table personalization rule .md file and issue warning text changes look good, unit tests for the rule updated accordingly, changeset added. Also checked text updates in README.md, all good from my side.

marufrasully

Please correct change set

AlinaGovoruhina

Reapproving after changeset changed to patch.

JannaLisa

please check my comments. Some of them (Currently, column, filter, ...) need to be applied further below as well, please.

JannaLisa · 2026-03-18T14:45:16Z


-Every table `personalization` property is correctly set to `true`.
-Omitting table personalization from the `manifest.json` table settings is also correct, because all personalization settings are provided by default for all tables.
+Every table `personalization` property is set to `true`. Omitting table personalization from the `manifest.json` table settings is also correct, because all personalization settings are provided by default for all tables.


I must really desperately need a vacation, because after reading the lines above, this statement doesn't really make sense to me

Suggested change

Every table `personalization` property is set to `true`. Omitting table personalization from the `manifest.json` table settings is also correct, because all personalization settings are provided by default for all tables.

Every `personalization` subproperty is set to `true` so all table settings are enabled.

Omitting the `personalization` property from the `manifest.json` file is also correct, because all personalization settings are enabled by default.

@JannaLisa, is this clearer?

JannaLisa · 2026-03-18T14:47:06Z

+            [TABLE_PERSONALIZATION_SORT]: 'Table data sorting must be enabled.',
            [TABLE_PERSONALIZATION_GROUP]:
-                'Table data grouping should be enabled for analytical and responsive type tables.',
+                'Table data grouping must be enabled for analytical and responsive type tables.',


Suggested change

'Table data grouping must be enabled for analytical and responsive type tables.',

'Table data grouping must be enabled for analytical and responsive table types.',

Suggested change

'Table data grouping must be enabled for analytical and responsive type tables.',

'Table data grouping must be enabled for analytical and responsive tables.',

JannaLisa · 2026-03-18T14:47:46Z

+                'Table data grouping must be enabled for analytical and responsive type tables.',
            [MISSING_PERSONALIZATION_PROPERTIES]:
-                'In case of using an object, omitting a setting is treated as false. {{undefinedPropertiesString}}.'
+                'When using an object, omitting a setting is treated as false. {{undefinedPropertiesString}}.'


when using which object?

Suggested change

'When using an object, omitting a setting is treated as false. {{undefinedPropertiesString}}.'

'When using the personalization object, omitting a property is treated as false. {{undefinedPropertiesString}}.'

JannaLisa · 2026-03-18T14:48:34Z

                        column: 21,
                        message:
-                            'In case of using an object, omitting a setting is treated as false. Currently column, filter, group, sort are disabled.'
+                            'When using an object, omitting a setting is treated as false. Currently column, filter, group, sort are disabled.'


Suggested change

'When using an object, omitting a setting is treated as false. Currently column, filter, group, sort are disabled.'

'When using an object, omitting a setting is treated as false. Currently, column, filter, group, and sort are disabled.'

This is in a test file so will be updated by a developer to match the finished proposal of what is in the file above.

Co-authored-by: JannaLisa <113453165+JannaLisa@users.noreply.github.com>

lfindlaysap · 2026-03-19T14:49:07Z

Suggested change

undefinedPropertiesString = `The ${diagnostic.undefinedProperties.join(', ')} ${diagnostic.undefinedProperties.length === 1 ? 'property is disabled' : 'properties are disabled'}`;

@AlinaGovoruhina, love the conditional logic for one versus multiple properties. 🚀 Is it possible to remove the comma from the first word and possibly have some logic for the last entry so it would read:

The column property is disabled.

or
The column, filter, and sort properties are disabled.

I updated the logic for the warning + the rule unit tests in bd06e27 Please check.

…ization-text-review

lfindlaysap

@AlinaGovoruhina, awesome 😃. Is it possible for this comma to be added? Just before the and?

lfindlaysap · 2026-03-20T10:02:33Z

                        column: 21,
                        message:
-                            'In case of using an object, omitting a setting is treated as false. Currently column, filter, group, sort are disabled.'
+                            'When using an object, omitting a setting is treated as false. The column, filter, group and sort properties are disabled.'


Suggested change

'When using an object, omitting a setting is treated as false. The column, filter, group and sort properties are disabled.'

'When using an object, omitting a setting is treated as false. The column, filter, group, and sort properties are disabled.'

@AlinaGovoruhina

Updated in 08b6bec

sonarqubecloud · 2026-03-20T13:03:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Reviewed texts

0f4084a

lfindlaysap requested a review from AlinaGovoruhina March 18, 2026 10:42

lfindlaysap requested a review from a team as a code owner March 18, 2026 10:42

lfindlaysap added the eslint-plugin-fiori-tools @sap-ux/eslint-plugin-fiori-tools label Mar 18, 2026

AlinaGovoruhina added 3 commits March 18, 2026 13:15

test: update text in unit tests

8e9b8ed

fix: typo

e353890

chore: add changeset

8eabf53

AlinaGovoruhina previously approved these changes Mar 18, 2026

View reviewed changes

lfindlaysap requested a review from JannaLisa March 18, 2026 13:28

hitesh-parmar reviewed Mar 18, 2026

View reviewed changes

Comment thread .changeset/clear-symbols-smile.md Outdated

marufrasully requested changes Mar 18, 2026

View reviewed changes

fix: changeset

21ebfd6

AlinaGovoruhina dismissed their stale review via 21ebfd6 March 18, 2026 13:47

marufrasully previously approved these changes Mar 18, 2026

View reviewed changes

Merge branch 'main' into eslint-table-personalization-text-review

06eec02

AlinaGovoruhina previously approved these changes Mar 18, 2026

View reviewed changes

JannaLisa reviewed Mar 18, 2026

View reviewed changes

Apply suggestions from code review

4b5ebf7

Co-authored-by: JannaLisa <113453165+JannaLisa@users.noreply.github.com>

lfindlaysap dismissed stale reviews from AlinaGovoruhina and marufrasully via 4b5ebf7 March 18, 2026 15:08

lfindlaysap requested a review from a team as a code owner March 18, 2026 15:08

lfindlaysap commented Mar 19, 2026

View reviewed changes

AlinaGovoruhina added 2 commits March 20, 2026 09:52

fix: properties warning message

bd06e27

Merge remote-tracking branch 'origin/main' into eslint-table-personal…

f27fa08

…ization-text-review

lfindlaysap commented Mar 20, 2026

View reviewed changes

fix: warning test

08b6bec

AlinaGovoruhina requested review from JannaLisa and hitesh-parmar March 26, 2026 07:11

AlinaGovoruhina requested a review from marufrasully March 26, 2026 07:11

-Every table `personalization` property is set to `true`. Omitting table personalization from the `manifest.json` table settings is also correct, because all personalization settings are provided by default for all tables.
+Every `personalization` subproperty is set to `true` so all table settings are enabled.
+Omitting the `personalization` property from the `manifest.json` file is also correct, because all personalization settings are enabled by default.

	'Table data grouping must be enabled for analytical and responsive type tables.',
	'Table data grouping must be enabled for analytical and responsive table types.',

	'When using an object, omitting a setting is treated as false. {{undefinedPropertiesString}}.'
	'When using the personalization object, omitting a property is treated as false. {{undefinedPropertiesString}}.'


	undefinedPropertiesString = `The ${diagnostic.undefinedProperties.join(', ')} ${diagnostic.undefinedProperties.length === 1 ? 'property is disabled' : 'properties are disabled'}`;

Conversation

lfindlaysap commented Mar 18, 2026

Uh oh!

changeset-bot bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

AlinaGovoruhina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marufrasully left a comment

Choose a reason for hiding this comment

Uh oh!

AlinaGovoruhina left a comment

Choose a reason for hiding this comment

Uh oh!

JannaLisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfindlaysap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 20, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

changeset-bot bot commented Mar 18, 2026 •

edited

Loading