Skip to content

Conversation

@nilshansen94
Copy link
Member

The uml component was organised into an own module and turned into a pure component. The module contains the pure component that should always behave in the same way with the same input. The uml container component injects the uml service and hands over the data from the service via an async pipe to the pure component.

Another terminology is the one of smart and dumb components. A dumb component is a pure (or presentational) component, a smart component is a container component.

@nilshansen94 nilshansen94 requested a review from vogti May 7, 2022 17:33
@nilshansen94 nilshansen94 self-assigned this May 7, 2022
@vogti vogti requested review from datomo and isabelge May 8, 2022 16:06
@Pipe({ name: 'value' })
export class ValuePipe implements PipeTransform {
transform(value: any): any {
if(!value){

Choose a reason for hiding this comment

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

I would turn this to use a conditional operator !value ? value : Object.values(value)

const getNameValidator = (required: boolean = false) => {
if (required) {
return [Validators.pattern('^[a-zA-Z_][a-zA-Z0-9_]*$'), Validators.required, Validators.max(100)];
} else {

Choose a reason for hiding this comment

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

The else here is not necessary, right?

}

const invalidNameMessage = (type: string = '') => {
type = type + ' ';

Choose a reason for hiding this comment

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

Modifying an input parameter?

if (name === '') {
return '';
}
else if (regex.test(name) && name.length <= 100) {

Choose a reason for hiding this comment

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

Flatten the conditions, what is worse then else is else if ;)

templateUrl: './uml.component.html',
styleUrls: ['./uml.component.scss']
styleUrls: ['./uml.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush

Choose a reason for hiding this comment

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

<3

$(e.source.element.nativeElement).css('z-index', 9000);
}

onDragging(){

Choose a reason for hiding this comment

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

Did you check the performance? I could imagine that the change detection goes nuts here. I would prefer a directive to manipulate the position of the element, but probably it would be a bit of work to refactore it.

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.

3 participants