Thanks, @Saira-A. I don't claim to have done a particularly thorough review, but a few small suggestions below.

We should talk about code reviews and PR handling at the next Community Call, to see what strategy we can come up with to keep things moving while @edsilv is busy elsewhere. I unfortunately don't have a lot of time to keep on top of UV things, but I'll at least try to help a little when I can. :-)

See below for a few more thoughts and minor nitpicks.

Beyond that, though, there seems to be a bigger issue. I definitely like what you've done with 1-up labels, but it seems that 2-up mode has somehow gotten broken in the process. When I look at the original test case, it comes up in 1-up mode and I don't see a way to switch it to 2-up.

Here's a link for convenience:

https://universalviewer-git-fork-saira-a-758-mnemoscene.vercel.app/#?xywh=-1538%2C0%2C11578%2C7617&iiifManifestId=https%3A%2F%2Fcollections.st-andrews.ac.uk%2F762295%2Fmanifest

Thanks, @Saira-A! One other thing we should do before we merge this is get the new language string ("Truncate Thumbnail Labels") translated into all the supported languages. Can @erinburnand help coordinate that?

@Saira-A, I'm a bit confused about the tests...

When I check out the current dev branch and run npm test, I get this:

> universalviewer@4.0.25 test
> jest

 PASS  __tests__/test.js (8.196 s)
 PASS  src/content-handlers/iiif/PubSub.spec.ts (9.138 s)
 PASS  src/content-handlers/iiif/modules/uv-shared-module/XYWHFragment.spec.ts (9.051 s)
 PASS  src/Utils.spec.ts (7.086 s)

Test Suites: 1 skipped, 4 passed, 4 of 5 total
Tests:       3 skipped, 7 passed, 10 total
Snapshots:   0 total
Time:        15.749 s, estimated 18 s
Ran all test suites.

When I switch to this branch and run the test suite, I get this:

Test Suites: 1 failed, 1 skipped, 3 passed, 4 of 5 total
Tests:       4 failed, 3 skipped, 5 passed, 12 total
Snapshots:   0 total
Time:        13.121 s

I notice that you've switched the tests to use port 8080 instead of port 4444. If I change that back, I then get more successes:

 PASS  src/content-handlers/iiif/PubSub.spec.ts (10.133 s)
 PASS  src/Utils.spec.ts
 PASS  src/content-handlers/iiif/modules/uv-shared-module/XYWHFragment.spec.ts (9.907 s)
 FAIL  __tests__/test.js (16.937 s)
  ● loads the viewer images

    thrown: "Exceeded timeout of 5000 ms for a test.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

      63 |
      64 |
    > 65 | it('loads the viewer images', async () => {
         | ^
      66 |   await page.waitForSelector('#thumb-0');
      67 |   const imageSrc = await page.$eval('#thumb-0 img', e => e.src);
      68 |   expect(imageSrc).toEqual(

      at Object.<anonymous> (__tests__/test.js:65:1)

Test Suites: 1 failed, 1 skipped, 3 passed, 4 of 5 total
Tests:       1 failed, 3 skipped, 8 passed, 12 total
Snapshots:   0 total
Time:        17.345 s
Ran all test suites.

I'm a little bit confused about all this, but I think I understand at least part of what is happening... there is some documentation which suggests that you need to run npm start before running the tests. That's going to put things up on port 8080, and I'm guessing that's why you changed the tests. However, in jest-puppeteer.config.js, the test suite is set up to run its own server on port 4444, which is presumably why things work in the dev branch even without starting anything up. The difference is that with npm start the code will automatically rebuild, and without it, you need to explicitly run npm run build to cause the latest changes to be tested.

So one issue seems to be that we need to decide on a single approach and configure/document everything accordingly!

In any case, it seems that the "loads the viewer images" test is commented out in the dev branch, so maybe we should leave it commented out here for now. I think fixing that problem is a separate issue.

I think if you comment out the failing test and change the port back from 8080 to 4444, we'll have this in a state where it can be merged safely, and we can figure out the rest through community conversation. What do you think?

Thanks for the progress, @Saira-A -- we're nearly done, but I'm having trouble with the tests. They're consistently failing in my environment, plus see below for a question about the commented-out "load the images" test.

I think this will be easier to manage if we can run tests as part of the GitHub process, so I've opened #1240 -- this will give us a common baseline so we're all running tests in the same environment, as well as catching changes that cause test failures. Maybe getting that merged into this PR will make further work here easier.

What do you think?

Thanks, @Saira-A -- tests are passing for me locally now, and I've merged the dev branch into this PR to get the GitHub Actions updates in, and things are passing there as well.

I'm approving this, but I won't merge yet just in case you have any further changes you wish to make (or in case @LanieOkorodudu wants to double-check anything). If you're happy, though, please feel free to hit merge!