Split JS record logic #4943
Split JS record logic #4943
Conversation
demiankatz
added
the
architecture
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ThoWagen! I haven't carefully reviewed the actual Javascript splitting yet, but tests are passing and the general setup seems reasonable. See below for a couple of smaller questions; I'll review the JS changes more carefully as soon as time permits!
module/VuFind/tests/unit-tests/src/VuFindTest/Config/UpgradeTest.php
Outdated
Show resolved
Hide resolved
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
It looks like there are now some conflicts here that need to be resolved -- I would guess #4903 is the cause.
Resolved |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ThoWagen, I found time for a more thorough review of the refactoring today and found only one issue -- see below:
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ThoWagen, I think this is safe to merge now. When you get a chance, do you mind reviewing #4961 and resolving any conflicts between this merge and #4901? I think it would be helpful to have all these pieces merged together so we have maximum test coverage and clarity when continuing to review the tab refactoring.
2 participants
As suggested here #4901 (review), I created this extra PR for just splitting the JS record code into multiple files.