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

Mccalluc/previews to resources #2121

Merged
merged 10 commits into from
Sep 13, 2021
Merged

Mccalluc/previews to resources #2121

merged 10 commits into from
Sep 13, 2021

Conversation

mccalluc
Copy link
Contributor

@mccalluc mccalluc commented Sep 2, 2021

Fix #2120. The effect on the screen is negligible (change the menu title; prefix options with "preview") but it turned into a bigger refactor: Lots of renaming, and then it turned out that the Azimuth link wasn't beeing included in the narrow view, so move it inside the component, and then since the same pattern repeats 3 times, factor out just the parts that change between menus.

@mccalluc mccalluc requested a review from john-conroy September 2, 2021 19:35
@mccalluc
Copy link
Contributor Author

mccalluc commented Sep 10, 2021

@john-conroy -- After the merge from main, there was one link that changed, but there was still another Cypress error I didn't understand: The general idea is that an element is on the screen, so cypress starts working with it, but then its visibility changes, and chained methods fail. This kludge is suggested towards the end of a long thread. I did try upgrading Cypress, but that didn't fix anything. (Upgrading might still be good, but I didn't want to muddy the waters in this PR.) Re-review?

@@ -24,7 +24,8 @@ describe('file-based-routes', () => {
// TODO: When we link to it from the menu, follow the link instead.
cy.visit('/publication');
cy.contains('Blue B. Lake');
cy.contains('An atlas of healthy and injured cell states').click();
// be.visible is a hack: https://github.com/cypress-io/cypress/issues/695
cy.contains('An atlas of healthy and injured cell states').should('be.visible').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this possibly considered 'not visible' before the hack because the test string provided isn't an exact match of what's on the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean it's just a substring? I don't think that's a problem, but let me check...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, even if it's the full string, the failure is the same:

-      cy.contains('An atlas of healthy and injured cell states').should('be.visible').click();
+      cy.contains('An atlas of healthy and injured cell states and niches in the human kidney').click();

Good to confirm, though.

@mccalluc mccalluc merged commit 7f33c4d into main Sep 13, 2021
@mccalluc mccalluc deleted the mccalluc/previews-to-resources branch September 13, 2021 20:14
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.

Change "previews" to "resources"
2 participants