Skip to content

Conversation

@mbeisel
Copy link
Contributor

@mbeisel mbeisel commented Jan 7, 2021

Fully test graph functionality (a lot of bugs got fixed and features added)
Added edit icon/name (requires backend commit for namechange)
MAP Import support
Fixed add new pattern issue, where the markdown wouldnt get loaded
Check readthedocs

@mbeisel mbeisel requested a review from manuwei January 7, 2021 17:24
@ghost
Copy link

ghost commented Jan 7, 2021

DeepCode's analysis on #d29371 found:

  • ℹ️ 1 minor issue. 👇
  • ✔️ 1 issue was fixed.

Top issues

Description Example fixes
Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type. Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@mbeisel mbeisel linked an issue Jan 8, 2021 that may be closed by this pull request
2 tasks
Copy link
Contributor

@manuwei manuwei left a comment

Choose a reason for hiding this comment

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

Looks good to me! I just added a few minor comments, mostly to improve overall code readability (not necessary written by you :) )

return this.getPatternLanguageByEncodedUri(uri);
}

getEdgeTypes(): Observable<string[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

a short description why this is not implemented for pattern languages would be helpful... Because there actually is a type for edges - do we not need it here? I think showing the type for pattern languages/views (as we do for solutions) would be nice

Copy link
Contributor

@manuwei manuwei left a comment

Choose a reason for hiding this comment

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

The PR looks fine to me :) I added a few minor comments, you can resolve them and merge afterwards

<ng-container *ngFor="let relation of directedPatternRelations">
<p class="horiz-centered">
<a [routerLink]="['..',relation.sourcePatternId]">{{ relation?.sourcePatternName}}</a>
<a [routerLink]="['..',relation.sourcePatternUri]">{{ relation?.sourcePatternName}}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it was the reason for the error when routing via patternlinks and accessing the pattern via the info button in the graph.
Instead of accessing the pattern via: patternpedia/PL/Patternuri patternpedia/PL/P_ID got called - which does not exist

return this.patternService.getPatternContentByPattern(this.patterns.find(it => it.id === toPattern)).pipe(
map((patterncontent) => {
const targetPatternContent = patterncontent.content;
// const targetPatternContent = patterncontent.content;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we delete these two lines ?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually we could delete the whole mapping? ( map((pattercontent)...

}
const dataSubscription = dataObservable.subscribe(() =>
this.cdr.detectChanges());
const dataSubscription = getPatternObservable.subscribe(() => dataObservable.subscribe(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

we should pipe those requests instead of subscribing in a subscribe...

@manuwei manuwei merged commit 614d000 into master Jan 25, 2021
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.

Add functionality to edit pattern name & icon Adding MAP Patterns to Patternatlas Deleted Patterns still exist in graph-view Graphview Issuelist

3 participants