Adds the service logo to column header and while schema building #6156
Conversation
github-actions
bot
added
the
reconciliation API design
main/webapp/modules/core/scripts/views/data-table/column-header-ui.js
Outdated
Show resolved
Hide resolved
|
The failing test looks unrelated to this PR, so I have opened an issue about it: #6158 |
| } | ||
| } | ||
| catch { | ||
| console.log("The URL is not valid"); |
There was a problem hiding this comment.
Those logging statements could be more useful with a more explicit error message. When you are looking at the code here, it is clear that this logging statement refers to a recon service URL, but imagine what happens when just seeing this message in the console, in the middle of other unrelated statements: the current error message does not give a lot of information, as we have no idea which URL is referred to.
Once the try { } catch { } block is part of the new function suggested above, this logging statement will be about the validity of the logo URL, hence we could instead have something like console.warn("Invalid logo URL supplied by service "+serviceUrl) or something like that.
There was a problem hiding this comment.
It seems like this problem is still there, no? Or do you see it resolved in another way?
|
@ayushrai206 if I remember correctly this PR isn't ready for merging yet, right? Or are you waiting for a review? |
Can you please review this one? |
There was a problem hiding this comment.
I tried it out like this:
- Reconcile a column to the test recon service
- The logo appears in the column header - amazing!
- In the Wikibase extension, start a new Wikidata schema
- The initialization of that UI does not complete - less amazing :-D
The console shows:
The URL is not valid 53 project-bundle.js:46590:13
Invalid logo URL supplied by service null 2 project-bundle.js:56864:13
Uncaught ReferenceError: serviceLogo is not defined
_createDraggableColumn http://127.0.0.1:3333/project-bundle.js:57275
updateColumns http://127.0.0.1:3333/project-bundle.js:57082
_rerenderTabs http://127.0.0.1:3333/project-bundle.js:57023
setUpTabs http://127.0.0.1:3333/project-bundle.js:56938
launch http://127.0.0.1:3333/project-bundle.js:57157
click http://127.0.0.1:3333/project-bundle.js:55972
createMenuItem http://127.0.0.1:3333/project-bundle.js:38411
dispatch http://127.0.0.1:3333/project-bundle.js:5147
handle http://127.0.0.1:3333/project-bundle.js:4951
| input.hide(); | ||
| input.val(""); | ||
| var columnDiv = $('<div></div>').appendTo(inputContainer); | ||
| column.appendTo(columnDiv); | ||
| //columnDiv.append(img); |
There was a problem hiding this comment.
This commented out code can probably be removed, no?
| var cell = $("<div></div>").addClass('wbs-draggable-column').text(name); | ||
| if(logo) { | ||
| var img =$("<img>"); | ||
| if(serviceLogo) { |
There was a problem hiding this comment.
This if block is probably redundant with the outer one, and seems to refer to an undeclared variable.
| } | ||
| } | ||
| catch { | ||
| console.log("The URL is not valid"); |
There was a problem hiding this comment.
It seems like this problem is still there, no? Or do you see it resolved in another way?
|
@ayushrai206 are you still working on this? I think we are really super close to something mergeable here. The three problems above should be fairly easy to solve :) |
|
Yesss!
…On Tue, 2 Apr 2024, 12:34 Antonin Delpeuch, ***@***.***> wrote:
@ayushrai206 <https://github.com/ayushrai206> are you still working on
this? I think we are really super close to something mergeable here. The
three problems above should be fairly easy to solve :)
—
Reply to this email directly, view it on GitHub
<#6156 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ARHBYIL3WNYJ7VYSAOHPSW3Y3JKAJAVCNFSM6AAAAAA7LNNOH6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRGIZDGOJUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fixes #4824
Changes proposed in this pull request: