CB-3703 save product configuration from ui #2334

yagudin10 · 2024-01-25T13:39:22Z

No description provided.

…e-settings-panel

sergeyteleshev · 2024-01-29T10:20:02Z

webapp/packages/core-administration/src/AdministrationSettingsService.ts

-  baseFeatures: [] as string[],
-};
+const settingsSchema = schema.object({
+  baseFeatures: schema.array(schema.string()).default([]),


should we use array of enums? so it is more typescript friendly

features is extendable entity, so we can't declare all possible values

webapp/packages/core-blocks/src/FormControls/Checkboxes/FieldCheckbox.tsx

sergeyteleshev · 2024-01-29T10:39:47Z

webapp/packages/core-blocks/src/FormControls/useCustomInputValidation.ts

+      if (typeof result === 'string') {
+        element.setCustomValidity(result);
+        return false;
+      } else {


no need for this else you can just go with

element.setCustomValidity(''); return true;

sergeyteleshev · 2024-01-29T10:44:43Z

webapp/packages/core-blocks/src/FormControls/useCustomInputValidation.ts

+    const result = validation(value);
+
+    try {
+      if (typeof result === 'string') {


what is expected behavior in case where result is an empty string? feels like in this case we should go in else statement

so I suggest to add extra check

if (typeof result === 'string' && result)

empty string will be treated as invalid field but with no message = no message must be treated as a bug (validation function must not return empty strings)

sergeyteleshev · 2024-01-29T10:48:56Z

webapp/packages/core-blocks/src/FormControls/useCustomInputValidation.ts

+      if (target.value === '') {
+        return;
+      }
+      if (target.validity.valid === true) {


why do we validate it if it is already valid?
probably, this should be
target.validity.valid === true

we want to validate on blur

browser validation focus first invalid field (so if it's invalid it will be impossible to focus other element)

sergeyteleshev · 2024-01-29T11:31:51Z

webapp/packages/core-plugin/src/SettingsManager/SettingsGroup.ts

+
+  constructor(readonly name: string, readonly parent?: SettingsGroup) {
+    this.id = uuid();
+    this.subGroups = [];


is strict order here required? may be we can go with Set (if not) or even create an OrderedSet like you did with OrderedMap (if yes). I just want this staff to be immutable so we don't mess up the settings and it has unique tabs and stuff

added better types

sergeyteleshev · 2024-01-29T11:40:32Z

webapp/packages/core-plugin/src/SettingsManager/SettingsManagerService.ts

+  settings: ScopeSettingsItem[];
+
+  constructor() {
+    this.settings = [];


same here for Set if you think it is a valuable idea

added better typing

sergeyteleshev · 2024-01-29T11:49:32Z

webapp/packages/core-server-localization/src/ServerLocalizationService.ts

+        })),
+      );
+    }
+  }


Should supported languages always be appeared? If so, I suggest to add notification here. so if there are no supported languages, we can find a bug once it appeared

we can find it by trying to switch to another language (we will see only english option)
actually we don't want to display any notifications to users because we will try to fall back to english and give users access to application
they will notice that language is not as should be in case they use other langoages than english

sergeyteleshev · 2024-01-29T12:33:47Z

webapp/packages/plugin-settings-panel/src/SettingsPanel/SettingsList.tsx

+
+  useEffect(() => {
+    if (ref.current) {
+      ref.current.addEventListener('scroll', e => {


please, add removeEventListener

sergeyteleshev · 2024-01-29T12:34:26Z

webapp/packages/plugin-settings-panel/src/SettingsPanel/SettingsList.tsx

+    list.push({ group, settings: groupSettings || [] });
+  }
+
+  useEffect(() => {


I would like to see this logic moved to the hook so it is easier to read this component

sergeyteleshev · 2024-01-29T13:14:17Z

also do yarn install for EE to update lock files with new lib

devnaumov · 2024-01-29T13:35:02Z

webapp/packages/core-plugin/src/PluginSettings.ts

    }

+    return schema.parse(undefined);


Why do we want to parse undefined here?

to get default value from schema (there no better way to do it)

what about schema.shape[name].default?

it's same as schema.string().default('ass') (shape returns schema object) we can't get default value this way

devnaumov · 2024-01-29T13:37:14Z

webapp/packages/core-plugin/src/SettingsManager/ISettingDescription.ts

+import type { SettingsGroup } from './SettingsGroup';
+
+export interface ISettingOptions {
+  id: string | number;


maybe we want to use one type for "id"?

devnaumov · 2024-01-29T14:11:41Z

webapp/packages/core-root/src/ServerSettingsService.ts

+    const changes = {};
+
+    for (const [key, value] of this.changes) {
+      setByPath(changes, key, value);


Probably we do not want to mutate object from util function

in this case it's expected, function accepts object, key and value and it will mutate object
we don't need immutable object here

devnaumov · 2024-01-29T14:20:59Z

webapp/packages/core-settings-localization/src/LocalizationSettingsManagerService.ts

+        options: this.localizationService.supportedLanguages.map(language => ({
+          id: language.isoCode,
+          name: language.displayName,
+        })) || [{ id: 'en', name: 'English' }],


Seems supportedLanguages are [] by default so the code will never fallback to the default object

const defaultOptions = [{ id: 'en', name: 'English' }]; const options = this.localizationService.supportedLanguages.length > 0 ? this.localizationService.supportedLanguages.map(language => ({ id: language.isoCode, name: language.displayName, })) : defaultOptions

fixed in other way, now this.localizationService.supportedLanguages must never be empty

devnaumov · 2024-01-29T14:29:49Z

webapp/packages/plugin-settings-administration/src/SettingsAdministration.tsx

+  const changed = serverSettingsService.isChanged;
+
+  return (
+    <Form context={form} style={{ height: '100%', display: 'flex', flexDirection: 'column' }}>


Could you move inline styles to the css module?

devnaumov · 2024-01-29T14:33:46Z

webapp/packages/core-plugin/src/SettingsManager/ISettingDescription.ts

+  type: ESettingsValueType;
+
+  group: SettingsGroup;
+  name: string;


TLocalizationToken instead of string for "name" and "description"

devnaumov · 2024-01-29T14:36:31Z

webapp/packages/plugin-settings-panel/src/SettingsPanel/SettingsGroups/SettingsGroups.tsx

+  }
+
+  function handleClick(id: string) {
+    document.querySelector('#' + getSettingGroupId(id))?.scrollIntoView();


Can we do it in react way using ref?

we can but it will be rocketsciense

devnaumov · 2024-01-29T14:36:47Z

webapp/packages/plugin-settings-panel/src/SettingsPanel/SettingsGroups/SettingsGroups.tsx

+
+export const SettingsGroups = observer<Props>(function SettingsGroups({ treeData }) {
+  function getNodeHeight(id: string) {
+    return 24;


const NODE_HEIGHT= 24;

there is no need (used once, keep simple)

…e-settings-panel

CB-4560 api for saving product configuration

5ac5c12

yagudin10 requested a review from alexander-skoblikov January 25, 2024 13:39

yagudin10 changed the title ~~CB-4560 api for saving product configuration~~ CB-3703 save product configuration from ui Jan 25, 2024

yagudin10 and others added 5 commits January 26, 2024 12:03

CB-4560 save product config in te

98f6e68

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

d2e8858

…e-settings-panel

CB-4560 move update product config to mutations

3917ebb

CB-3703 feat: extend base components

19bbe0c

CB-3703 feat: settings editor

21ce5c1

Wroud marked this pull request as ready for review January 29, 2024 09:30

sergeyteleshev requested review from sergeyteleshev and devnaumov January 29, 2024 10:00

sergeyteleshev reviewed Jan 29, 2024

View reviewed changes

devnaumov requested changes Jan 29, 2024

View reviewed changes

Wroud added 3 commits January 29, 2024 23:39

CB-3703 chore: review fixes

eeda015

CB-3703 chore: pr review

c8c5e7a

CB-3703 feat: update server configuration icon

1417621

sergeyteleshev self-requested a review January 30, 2024 09:30

sergeyteleshev approved these changes Jan 30, 2024

View reviewed changes

devnaumov approved these changes Jan 30, 2024

View reviewed changes

EvgeniaBzzz and others added 9 commits January 30, 2024 15:41

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

5dd0985

…e-settings-panel

CB-4560 add events

ff842c8

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

1afde6b

…e-settings-panel

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

6fdeffd

…e-settings-panel

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

76735c3

…e-settings-panel

CB-3703 fix: localization

2ae57b1

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

516f54b

…e-settings-panel

CB-3703 chore: localization

717b257

Merge branch 'devel' into CB-3703-ability-to-edit-configuration-in-th…

9220c25

…e-settings-panel

Wroud merged commit fdde434 into devel Jan 31, 2024
2 of 5 checks passed

Wroud deleted the CB-3703-ability-to-edit-configuration-in-the-settings-panel branch January 31, 2024 17:33

CB-3703 save product configuration from ui #2334

CB-3703 save product configuration from ui #2334

Conversation

yagudin10 commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyteleshev commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment