-
Notifications
You must be signed in to change notification settings - Fork 84
Load overrides in main.js #43
Load overrides in main.js #43
Conversation
Updated the PR to use asynchronous data loading. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't try yet, but if my code analysis correct, might need further tune up
assets/js/main.js
Outdated
poster["posters"].map( | ||
function (e, i) { | ||
Object.assign(e, overrides["posters"][i]); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @christian-monch ! Things are moving too fast,so it might need tune up now:
From my understanding of the logic here,
- if
i
is still and index within list, it would no longer work as of f6a88ee where I RFed code to accommodate an additional input SoftwareDemos.tsv. So I now in Python create a "proper" mapping based on the "number" field. So something similar should be done here - does
Object.assign(e, overrides["posters"][i])
"update" or "assigns" the override? overrides contain only overrides, not all the fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Did not see the updated mapping, was referring to the zip-version. Will change that, should be easy.
- How overrides work is understood. ;-) assign does an "update", i.e only fields in overrides["posters"][i] will be modified in e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GREAT!
Even though overrides became less important since we did populate majority of entries, I still think it would be useful, so I will keep an eye for the update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has been discussed and agreed upon. Equivalent code now in force-pushed commit 030d63a
and damn me -- I just missed this PR originally those many hours ago, sorry @christian-monch |
No worries :-) |
275e29d
to
030d63a
Compare
Hi @yarikoptic, just added a few lines to handle overrides in main.js. The other stuff still has to be done in the python script. As you say, it might not be relevant. If not, just close the PR |
success: function (overrides) { | ||
data.posters.map( | ||
function (e, i) { | ||
Object.assign(e, overrides.posters[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one still just uses [i]
instead of creating a proper mapping table -- it would fail for entries which are not in overrides (e.g. software demos)
I had to fix related issue on this PR #79 .. and I believe it fixed this issue described here.. sorry for stepping on each other's toes! |
https://github.com/datalad-datasets/ohbm2020-posters/pull/79/files#diff-1553c69c86f6c7a09f8c622bcea5deefR268 provided alternative implementation, so closing |
This tackles issue #4
A first approach (my javascript skills suck) to process overrides in main.js. It should work though.