NAS-128557 / 24.10 / Finishing WidgetGroupFormComponent #10022
Conversation
bugclerk
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10022 +/- ##
===========================================
- Coverage 73.86% 4.86% -69.01%
===========================================
Files 1520 1526 +6
Lines 53202 53447 +245
Branches 6352 6387 +35
===========================================
- Hits 39296 2598 -36698
- Misses 13906 50849 +36943 ☔ View full report in Codecov by Sentry. |
| [disabled]="(widgetGroupFormStore.hasValidationErrors$ | async)" | ||
| (click)="onSubmit()" | ||
| >{{ 'Save' | translate }}</button> | ||
| </ix-form-actions> | ||
|
|
||
| <!-- TODO: Category dropdown, pull widget definitions from widgetRegistry and regroup by category --> |
There was a problem hiding this comment.
You can remove the todos now.
| this.setCategoryOptions(); | ||
| } | ||
|
|
||
| getEnumKeyByEnumValue<T extends Record<string, string>>( |
There was a problem hiding this comment.
This will lead to untranslatable categories. You should just use widgetCategoryLabels.
| }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a fan of this design.
You're mixing two types of communication. You have both communication through inputs and through the store. This also makes settings component know about things like store and slotIndex, when it doesn't really need to know this. Inheritance also makes it harder to navigate the code.
I'd rather we communicate via inputs and outputs here without additional dependencies and an abstract class.
If there is an issue with outputs in componentOutlet, use https://angular.io/api/core/ViewContainerRef#createComponent or pass-in a subject.
|
|
||
| @UntilDestroy() | ||
| @Directive() | ||
| export abstract class WidgetSettingsDirective<Settings extends SomeWidgetSettings = null> implements OnInit { |
There was a problem hiding this comment.
It can still be called WidgetSettingsComponent.
| export class WidgetGroupFormStore extends ComponentStore<WidgetGroupFormState> { | ||
| readonly layout$ = this.select((state) => state.layout); | ||
| readonly hasValidationErrors$ = this.select((state) => { | ||
| for (const slot of state.slots) { |
There was a problem hiding this comment.
return state.slots.some((slot) => slot.validationErrors);
| }; | ||
| }); | ||
|
|
||
| getSlotConfig(slotIndex: number): SlotConfig { |
| }); | ||
|
|
||
| this.widgetTypesOptions$ = of(sizeSuitedTypes.map((type) => { | ||
| return { label: this.getEnumKeyByEnumValue(WidgetType, type), value: type }; |
There was a problem hiding this comment.
Name should come from widgetDefinition and be translated.
| return keys.length > 0 ? keys[0] : null; | ||
| } | ||
|
|
||
| setCategoryOptions(): void { |
There was a problem hiding this comment.
This is complicated and you are doing similar things here and in setWidgetTypeOptions.
Instead, why not first filter for widgets that fit the slot size in a private method:
const widgets = Object.values(widgetRegistry);
const layout = this.layout.value;
const slotSize = layoutToSlotSizes[layout][this.selectedSlot];
const supportedWidgets = widgets.filter((widget) => widget.supportedSizes.includes(slotSize));and then extract categories:
const categories = uniq(supportedWidgets.map((widget) => widget.category));
return categories.map((category) => {
return {
label: widgetCategoryLabels.get(category) || category,
value: category
};
});or types when you need them.
|
|
||
| // TODO: Implement template options | ||
| templateOptions$ = of([]); | ||
|
|
||
| constructor( | ||
| private formBuilder: FormBuilder, | ||
| private chainedRef: ChainedRef<WidgetGroup>, | ||
| protected chainedRef: ChainedRef<WidgetGroup>, |
There was a problem hiding this comment.
Editing existing widget groups should be supported.
| }); | ||
| readonly layoutsMap = widgetGroupIcons; | ||
|
|
||
| protected readonly template = new FormControl(''); |
There was a problem hiding this comment.
I think component store makes your life harder here, because instead of having one source of truth you have things stored in three places. There are form controls, there's group inside of the component and there's a store.
It's really hard to trace what changes what here.
Having a separate place for logic is a good idea, but then we should come up with a way to utilize it more.
We should avoid doing what we did in pool creation wizard, because it really makes for some complicated logic.
Options for dropdowns should react to changes in data store whereever it is.
See this video https://www.youtube.com/watch?v=skOTEbGwncE
It focuses on rxjs, but you could do the same thing easier with signals.
You need to pick one place for source of truth (could be a store, could be a form or signals here or in a service) and then have other things derive from it.
|
Also you are storing validation errors for every slot, which is fine, but then we need to come up with an error state for those widgets, so that user knows where the error is if he switches to another slot. |
|
Will add tests once the approach is approved. |
| @@ -75,7 +75,7 @@ export class DashboardComponent implements OnInit { | |||
| protected onAddGroup(): void { | |||
| const newGroup: WidgetGroup = { | |||
| layout: WidgetGroupLayout.Full, | |||
| slots: [], | |||
| slots: [{ category: null, type: null }], | |||
There was a problem hiding this comment.
This feels like a behaviour specific to WidgetGroupFormComponent.
There was a problem hiding this comment.
Technically, we should be adding a new group here at all. But we're doing that and we are adding the new group to the dashboard right here. But since, we're doing that, a full layout widget has at least 1 slot and the object should represent that. I will remove the whole logic from here.
| }); | ||
| readonly layoutsMap = widgetGroupIcons; | ||
|
|
||
| get hasErrors(): boolean { |
There was a problem hiding this comment.
This should be a computed signal.
|
|
||
| setTypeOptions(category: WidgetCategory): void { | ||
| this.form.controls.type.setValue(null); | ||
| const layoutSupportedWidgets = this.getLayoutSupportedWidgets() as Widget[]; |
There was a problem hiding this comment.
as Widget[] is incorrect here.
You could try introducing an UntypedWidgetDefinition similar to WidgetDefinition, but without generics to return in getLayoutSupportedWidgets.
Or if you only care about smaller set of fields, you can introduce a new interface with just those fields locally.
| this.refreshSettingsContainer(); | ||
| } | ||
|
|
||
| setCategoryOptions(): void { |
There was a problem hiding this comment.
Any chance of turning some of those methods that set options into computed signals?
| @@ -13,6 +16,13 @@ export enum WidgetType { | |||
| Cpu = 'cpu', | |||
| } | |||
|
|
|||
| export const widgetTypeLabels = new Map<WidgetType, string>([ | |||
There was a problem hiding this comment.
Use name field in widgetDefinition is for this.
| @@ -21,6 +31,7 @@ export enum SlotSize { | |||
|
|
|||
| export interface Widget { | |||
| type: WidgetType; | |||
| category: WidgetCategory; | |||
There was a problem hiding this comment.
Shouldn't be part of Widget interface. We are storing widget settings in user attributes on middleware and we shouldn't store category there, because there is no need to tightly couple the connection there.
| type WidgetSettingsComponentType<Settings> = Settings extends SomeWidgetSettings | ||
| ? WidgetSettingsComponent<Settings> | ||
| ? Settings |
There was a problem hiding this comment.
The original intent was to tie widget settings to widgets, so that developers don't make mistakes when they define their components.
This is now in broken state.
You need to:
- Make
WidgetSettingsRefaccept aSettingsgeneric. - Recover back
WidgetSettingsComponentand have it requireWidgetSettingsRef<Settings>as one of the fields (instead ofsomething). - Have
WidgetInterfaceIpSettingsComponentimplementWidgetSettingsComponentcorrectly.
| import { ValidationErrors } from '@angular/forms'; | ||
|
|
||
| export class WidgetSettingsRef { | ||
| getData: () => { slot: number; settings: object }; |
There was a problem hiding this comment.
I don't really see why settings components need to know what slot number they are in.
You can add slot number to WidgetSettingsRef in a way that would hide this information or you can just rely on currently selected slot.
| }); | ||
| } | ||
|
|
||
| private getAllFormErrors(): Record<string, ValidationErrors> { |
There was a problem hiding this comment.
Needs to go to some utils function or be part of WidgetSettingsRef class.
| } | ||
|
|
||
| getLayoutSupportedWidgets(): { type: WidgetType; category: WidgetCategory; [key: string]: unknown }[] { | ||
| const widgetsEntires = Object.entries(widgetRegistry); |
|
Looked into failing tests a little bit. and then to start removing parts of template / component to see where the issue is. My suspicion is that Also It would be good to update other tests to also test what user sees / actual behaviour of the component, instead of calling component functions that should be marked as |
RehanY147
force-pushed
the
NAS-128557
branch
2 times, most recently
from
29ce6b1
to
fad2f76
Compare
|
This PR has been merged and conversations have been locked. |
No description provided.