Skip to content
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

M4 update #67

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

M4 update #67

wants to merge 7 commits into from

Conversation

gerdesque
Copy link

No description provided.

expect(wrapper.find(Link).at(0).props().children).toEqual('Whole image (4000 x 1000px)');
expect(wrapper.find(Link).at(1).props().children).toEqual('Whole image (2000 x 500px)');
expect(wrapper.find(Link).at(2).props().children).toEqual('Whole image (1000 x 250px)');
// TODO: Fix the tests that fails because of OSDReferences.get(windowId).current.viewport
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails due to a change in behaviour in https://github.com/ProjectMirador/mirador/blob/37b1f21a4b90a257b870f65560b6274b1a982422/src/plugins/OSDReferences.js and needs to be investigated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test is missing some mocked values, in particular currentBoundsSpy.mockImplementation() with a return value like in the base version:

getBounds: () => ({
x: 0, y: 0, width: 4000, height: 1000,
}),

"build:umd": "NODE_ENV=production webpack --mode=production",
"build:es": "mkdir -p es && cp -r src/* es && NODE_ENV=production MODULE_FORMAT=es npx babel es -d es",
"build:cjs": "mkdir -p lib && cp -r src/* lib && NODE_ENV=production MODULE_FORMAT=cjs npx babel lib -d lib",
"parcel": "parcel demo/src/index.html",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This PR Update plugin for Mirador 4 mirador-image-tools#49 and the main Mirador repo use webpack instead of parcel to serve the content. While I don't have a strong preference about this and we deferred changes like introducing vite, I think we should try to use consistent approaches across the different repos. Is there a particular reason for using parcel here, even though webpack is already used for the build scripts?

  2. Similar thing about script names: it would be nice to be able to start every application with npm run start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants