Skip to content

Comments

Knowledge graph UI test#100

Open
HansRichard wants to merge 7 commits intodevelopmentfrom
PermissionTest
Open

Knowledge graph UI test#100
HansRichard wants to merge 7 commits intodevelopmentfrom
PermissionTest

Conversation

@HansRichard
Copy link

No description provided.

Gruppe C test
This reverts commit cc33aea.
RuneAagaard
RuneAagaard previously approved these changes Dec 6, 2022
Copy link
Contributor

@KasperHenningsen KasperHenningsen left a comment

Choose a reason for hiding this comment

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

Har lige et par kommentare.
I må bare skrive hvis i er uenige 😁

Comment on lines 17 to 27
function Search(id) {
document.getElementById('table').innerHTML = "";
document.getElementById('svg-body').innerHTML = "";

const xhr = new XMLHttpRequest();
xhr.open('GET', 'http://localhost:3030/SNOMEDTEST?query=PREFIX+skos%3A%3Chttp%3A%2F%2Fwww.w3.org%2F2004%2F02%2Fskos%2Fcore%23%3ESELECT+%3FSUBJECT+%3FPREDICATE+%3FOBJECT+where%7B%0A++%3Chttp%3A%2F%2Fwww.example.org%2F'+id+'%3E+(skos%3A%7C!skos%3A)%7B0%7D+%3FSUBJECT.%0A++%3FSUBJECT+%3FPREDICATE+%3FOBJECT+%0A%7DLIMIT+150%0A', true);
xhr.withCredentials = true;
xhr.send(null);
xhr.onloadend = (event) => {ResponseToTable(xhr.response)};

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Kan anbefale at følge 'Service pattern'. der er eksempler på dem i /knox-ui/Services/

document.getElementById('svg-body').innerHTML = "";

const xhr = new XMLHttpRequest();
xhr.open('GET', 'http://localhost:3030/SNOMEDTEST?query=PREFIX+skos%3A%3Chttp%3A%2F%2Fwww.w3.org%2F2004%2F02%2Fskos%2Fcore%23%3ESELECT+%3FSUBJECT+%3FPREDICATE+%3FOBJECT+where%7B%0A++%3Chttp%3A%2F%2Fwww.example.org%2F'+id+'%3E+(skos%3A%7C!skos%3A)%7B0%7D+%3FSUBJECT.%0A++%3FSUBJECT+%3FPREDICATE+%3FOBJECT+%0A%7DLIMIT+150%0A', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I må meget gerne flytte links ud i .env filen, så skal man ikke rette links i koden hvis de en gang skulle ændre sig, eller hvis man vil køre UI'en lokalt.

så kan man lave en '.env.local' når det skal køres localt og en '.env.production' når det skal køres på serveren.

React app er lidt anderledes når man tilføjer env variabler, de skal følge REACT_APP_[ENV_VAR_NAME]
(i kan se på /knox-ui/Services/ hvis det ikke er helt klart)

Comment on lines +230 to +232
function id(){
return "kl";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Der er ingen grund til at bruge 'function' hvis den alligevel altid returnere det samme.

Suggested change
function id(){
return "kl";
}
const id = "kl";

var force = d3.layout.force().size([1000, 500]);

var graph = triplesToGraph(svg);
console.log(graph);
Copy link
Contributor

Choose a reason for hiding this comment

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

fjern gerne console.log, det høre ikke til i production.

Suggested change
console.log(graph);

Comment on lines 147 to 148
//linkTexts.append("title")
// .text(function(d) { return d.predicate; });
Copy link
Contributor

Choose a reason for hiding this comment

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

Hvis dette ikke skal bruges må i gerne fjerne det.

Suggested change
//linkTexts.append("title")
// .text(function(d) { return d.predicate; });

Comment on lines 155 to 156
.call(force.drag)
;//nodes
Copy link
Contributor

@KasperHenningsen KasperHenningsen Dec 6, 2022

Choose a reason for hiding this comment

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

#100 (comment)

Suggested change
.call(force.drag)
;//nodes
.call(force.drag);

Comment on lines 163 to 164
.text( function (d) { return d.label; })
;
Copy link
Contributor

Choose a reason for hiding this comment

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

I har mange "floating" semikoloner.

Suggested change
.text( function (d) { return d.label; })
;
.text( function (d) { return d.label; });

Comment on lines 191 to 194
.attr("y", function(d) { return d.y + 3; })


;
Copy link
Contributor

@KasperHenningsen KasperHenningsen Dec 6, 2022

Choose a reason for hiding this comment

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

#100 (comment)

Suggested change
.attr("y", function(d) { return d.y + 3; })
;
.attr("y", function(d) { return d.y + 3; });

Comment on lines 199 to 201
.attr("y", function(d) { return 4 + (d.s.y + d.p.y + d.o.y)/3 ; })

;
Copy link
Contributor

@KasperHenningsen KasperHenningsen Dec 6, 2022

Choose a reason for hiding this comment

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

#100 (comment)

Suggested change
.attr("y", function(d) { return 4 + (d.s.y + d.p.y + d.o.y)/3 ; })
;
.attr("y", function(d) { return 4 + (d.s.y + d.p.y + d.o.y)/3 ; });

Comment on lines +210 to +211
.start()
;
Copy link
Contributor

@KasperHenningsen KasperHenningsen Dec 6, 2022

Choose a reason for hiding this comment

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

#100 (comment)

Suggested change
.start()
;
.start();

;

}
function hej(){
Copy link
Contributor

Choose a reason for hiding this comment

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

Hej 😄
Den burde have et andet navn før den bliver skubbet op 😄

Copy link
Contributor

@selectionator selectionator left a comment

Choose a reason for hiding this comment

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

Er enig i de ting Kasper også har sat kommentarer ved. Tænker at det kunne være dejligt at sørge for at koden er ryddet op inden den skubbes op på branchen 😊

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.

4 participants