comunica / jQuery-Widget.js

🖼 A jQuery widget to query heterogeneous interfaces using Comunica SPARQL
http://query.linkeddatafragments.org/
MIT License
18 stars 35 forks source link

Upgrade yasgui-yasqe to @triply/yasqe #116

Closed phivk closed 2 years ago

phivk commented 2 years ago

everything seems to work ok when I run yarn run dev and yarn run build, however there seems to be a MIME type issue with importing or applying the CSS file (which gets required in src/ldf-client-ui.js):

Refused to apply style from 'http://localhost:8080/styles/yasqe.css' because its MIME type ('text/html') is not a supported stylesheet MIME type, and strict MIME checking is enabled.

image

CLAassistant commented 2 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: phivk
:x: dependabot[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

rubensworks commented 2 years ago

The error message mentions yasqe.css, while yasqe.min.css is being imported here. Perhaps that is causing the problem?

phivk commented 2 years ago

I've updated yasqe.css to yasqe.min.css in /index.html, which resolved the MIME type errors.

One more thing I noticed is this warning when running yarn install:

warning " > @triply/yasqe@4.2.23" has unmet peer dependency "@triply/yasgui@4.x".

I believe the project runs fine without including "@triply/yasgui@4.x" as explicit dependency, but I wanted to flag this warning to see if you think it needs resolving before merging this PR...

rubensworks commented 2 years ago

I believe the project runs fine without including "@triply/yasgui@4.x" as explicit dependency, but I wanted to flag this warning to see if you think it needs resolving before merging this PR...

I also think it's fine to ignore the warning. But to make sure, just tagging @LaurensRietveld to see if we're correct in our assumption.

LaurensRietveld commented 2 years ago

Yep, you can ignore this :+1:

rubensworks commented 2 years ago

@phivk I unfortunately had to revert this merge, because it looks like some things are broken due to this change:

Before:

Screenshot 2022-06-20 at 13 41 27

After:

Screenshot 2022-06-20 at 13 41 20

Can you reproduce this on your system @phivk?

phivk commented 2 years ago

ouch... I am not sure what you mean by "The autocompletion of prefix appears to be missing", but visually my local setup looks identical (missing numbers)

image

I am not sure how much I can be of help in resolving this, but if someone has an idea I am happy to test things locally. @rubensworks what do you think would be a good way to move forward?

rubensworks commented 2 years ago

I am not sure what you mean by "The autocompletion of prefix appears to be missing"

For example, on http://query.linkeddatafragments.org/, if you type schema:, the proper prefix for schema.org will be automatically added. This seems to not be the case anymore.

I am not sure how much I can be of help in resolving this, but if someone has an idea I am happy to test things locally. @rubensworks what do you think would be a good way to move forward?

Also not sure what's the problem. I would have guessed that with the update, things would still just work.

But perhaps there are some config options that need to be explicitly enabled now. Perhaps the yasqe docs can help?

phivk commented 2 years ago

@rubensworks I finally got round to investigating further and had a closer look at the Yasgui documentation. This brings me to two points about upgrading the Yasgui / Yasqe dependency.

incremental update

Since Yasqe extends Codemirror, I tried using its config when YASQE gets instantiated by ensuring the default options are set (this includes showing line-numbers). See the change here: https://github.com/beeldengeluid/jQuery-Widget.js/commit/79ded9b2b79eaa87503e1067d4336715ca356de1

This fixes the display of line numbers, but not the prefix autocomplettion and I haven't been able to figure out what is preventing it to work.

structural upgrade

Looking at the Yasgui docs made me realise that the way the Yasgui and its child module Yasqe work, have significantly changed from the way Comunica's jQuery-Wdget is currently using the outdated Yasqe module.

The current implementation based on yasgui-yasqe defines UI components in index.html and augments their functionality in ldf-client-ui.js.

The way @triply/yasgui approaches this is by defining an empty div and targeting it within the Yasgui constructor, e.g. like so

const yasgui = new Yasgui(document.getElementById("yasgui"));

The UI can be further configured via options, but this would mean a significant rewrite of this project and I am not sure if that's a direction you want to take. I'd also have no bandwidth to take on such endeavour.

rubensworks commented 2 years ago

Thanks for following up on this @phivk!

I'm definitely open to the structural upgrade you suggest. I don't have the bandwidth myself either to take this up, but if this issue has priority for you, we could look into placing a bounty on this via the Comunica Association. We can then look for a freelancer to continue upon the work you already did so far.