Skip to content
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

Removed menu #54

Closed
wants to merge 3 commits into from
Closed

Removed menu #54

wants to merge 3 commits into from

Conversation

rosalieper
Copy link

No description provided.

The previous pullrequest(shexjs#48) was overwritten by (shexjs#46). This commit adds the change back
menu is removed and an about link added
@chukarave
Copy link

Looks good to me

Copy link
Collaborator

@lucaswerkmeister lucaswerkmeister left a comment

Choose a reason for hiding this comment

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

I don’t think the first two commits should be part of this pull request – I think your rmMenu branch might need a rebase on the upstream wikidata branch?

{queryStringParm: "interface", location: $("#interface"), deflt: "human" },
{queryStringParm: "regexpEngine", location: $("#regexpEngine"), deflt: "threaded-val-nerr" },
]);
var QueryParams = Getables.concat([]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The .concat() is unnecessary now, this could also just be var QueryParams = Getables; as far as I can tell.

@@ -229,33 +229,9 @@
<!-- </ul> -->
<form id="menuForm">
<button id="menu-button" title="click for menu">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think the “click for menu” title is accurate anymore, but I’m not sure.

@andrawaag
Copy link
Contributor

Did this also remove the result section? It seems it is now only possible to get a binary answer (True or False) but the necessary information to point to the actual location where the data does not fit the schema, Like here: https://colab.research.google.com/drive/1Y1Wv9TGVzEIe26vvXhQuxFUHXKqij-hk

@ericprud
Copy link
Contributor

I agree that removing the results section makes it basically impossible to diagnose errors.
I added a switch ?ui=full to enable the results section and the control menu. You can see it in use in a video. Does the behavior without that switch close this issue and PR?

@lucaswerkmeister
Copy link
Collaborator

I’m really confused now. Removing the results has nothing to do with this pull request, as far as I can tell, but was done in e0bca4c (#46). And I’m pretty sure we don’t that in the Wikidata case – I don’t even see the binary answer @andrawaag mentions, as far as I can tell there aren’t any results at all, which can’t be what we want.

@ericprud
Copy link
Contributor

They're only connected by the idea that both can be controlled through the mechansim i used to switch the [control] button on/off.

Re the advisability of turning off results, I agree. It's easy to re-enable them if that's what folks want.

@lucaswerkmeister
Copy link
Collaborator

Re the advisability of turning off results, I agree. It's easy to re-enable them if that's what folks want.

Yes, please do :) we’ll have to find another way to disable displaying the parsed schema without removing the results as well.

ericprud pushed a commit that referenced this pull request May 24, 2019
@ericprud
Copy link
Contributor

after extensive editing, i was able to fulfill this request

@rosalieper rosalieper closed this Jun 4, 2019
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.

None yet

5 participants