-
Notifications
You must be signed in to change notification settings - Fork 354
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
Support richer, more readable Javascript configuration #1863
Conversation
All tests are passing, so I think I've successfully extracted the appropriate code from #1799. @EreMaijala and @mtrojan-ub, can you give this a look and let me know if you have any concerns or if you think it's ready to merge? Also note that @mtrojan-ub is not currently a member of the vufind-org Developers group, but I think he should be so that he can be assigned as a reviewer, etc. I have sent an invitation! |
Thanks, @EreMaijala, for the feedback. I'm hoping @mtrojan-ub can answer your questions; I've added him as a reviewer as well so he'll be notified. :-) |
Thanks for inviting me to the vufind-org Developers group, i feel truly honored 😃 |
Of course, @mtrojan-ub. Hopefully nobody will be mad at me for adding you before the Project Management Committee is fully up and running, but you've clearly demonstrated that you understand the code base well, so I'm not worried. ;-) |
@EreMaijala / @mtrojan-ub, I think I've made all the changes discussed in the review above as part of commit 8ccbca2 (I also removed an unnecessary else to quiet a warning in my IDE). I don't have time to test any of this today -- but if you review that commit and it looks good to you, I'll run the full test suite in the near future, and merge if you're both happy and all tests pass. |
Okay, we're back to having the full test suite passing again. Any final thoughts before I merge, @mtrojan-ub or @EreMaijala? |
No objections from my side => i would be glad if we could merge this and make a step forward on this issue! |
@demiankatz No objections from me either. |
I noticed that the comments mentioned a load_before setting that is no longer supported, so I updated the documentation (with some other minor tweaks) in e103ddb. |
My comment changes didn't break the Travis build, so I'm going ahead and merging now. Thanks, everyone! I'll update #1799 in the near future as well. |
This PR extracts some functionality from #1799 in an effort to get the configuration improvements merged separately from other additional features.
TODO