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

Fix MUI5 testing errors #3793

Merged
merged 21 commits into from
Nov 9, 2023
Merged

Conversation

marlo-longley
Copy link
Member

No description provided.

@marlo-longley marlo-longley force-pushed the mui5-tests branch 2 times, most recently from b0df016 to 4a5bf0d Compare October 27, 2023 19:43
@marlo-longley marlo-longley changed the title WIP: working to eliminate testing errors Fix MUI5 testing errors Oct 27, 2023
@marlo-longley marlo-longley marked this pull request as ready for review October 27, 2023 21:09
Copy link
Contributor

@barmintor barmintor left a comment

Choose a reason for hiding this comment

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

I think there are two changes to consider in the comments but I don't think they should hold up merging this PR to the mui5 branch.

@@ -6,7 +6,8 @@ describe('Thumbnail navigation', () => {
await expect(page).toMatchElement('.mirador-window', { polling: 'mutation', timeout: 5000 });
});

it('navigates a manifest using thumbnail navigation', async () => {
//TODO: pick a new url; this Harvard one is 404
Copy link
Contributor

Choose a reason for hiding this comment

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

The manifest URL seems ok - is it required in Mirador for canvas IDs to be dereferenceable? It looks like that manifest has all the relevant canvas data inline.

@@ -138,6 +138,7 @@ export class ManifestListItem extends Component {
},
})}
data-manifestid={manifestId}
data-active={active}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the best way to handle this in mui5 is to make this a ListItemButton and use the selected attribute (which deprecated on ListItem but not ListItemButton).

@marlo-longley
Copy link
Member Author

Because of a migration, I had to add mui-x to our project to keep using Tree View:
8932afd

It is a paid product but is also free to use under MIT License, so I think we should. be good. But wanted to check on that.

@barmintor
Copy link
Contributor

@marlo-longley I came to the same conclusion for the React18 patches on our fork.

@phil-plencner-hl phil-plencner-hl self-requested a review November 9, 2023 22:51
Copy link
Collaborator

@phil-plencner-hl phil-plencner-hl left a comment

Choose a reason for hiding this comment

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

Ran npm run test and all tests pass. Approved.

@phil-plencner-hl phil-plencner-hl merged commit d348a06 into ProjectMirador:mui5 Nov 9, 2023
3 checks passed
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.

3 participants