-
Notifications
You must be signed in to change notification settings - Fork 22
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
Pure uml component #73
base: master
Are you sure you want to change the base?
Conversation
@@ -3,6 +3,9 @@ import {Pipe, PipeTransform} from '@angular/core'; | |||
@Pipe({ name: 'value' }) | |||
export class ValuePipe implements PipeTransform { | |||
transform(value: any): any { | |||
if(!value){ |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 + ' '; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 ;)
|
||
@Component({ | ||
selector: 'app-uml', | ||
templateUrl: './uml.component.html', | ||
styleUrls: ['./uml.component.scss'] | ||
styleUrls: ['./uml.component.scss'], | ||
changeDetection: ChangeDetectionStrategy.OnPush |
There was a problem hiding this comment.
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(){ |
There was a problem hiding this comment.
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.
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.