-
Notifications
You must be signed in to change notification settings - Fork 15
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
Reusable proptypes #1189
Reusable proptypes #1189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and certainly a plus for how we create and hydrate components!
I've left an additional comment on the new console check utility.
client/tests/e2e/util/index.js
Outdated
* @param {number} [timeout=500] - Timeout in milliseconds to wait for errors | ||
* @returns {Promise<string[]>} - Array of error messages, if any | ||
*/ | ||
async function checkConsoleErrors(page, action, timeout = 500) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super useful! This definitely gives a lot of confidence!
page, | ||
introductionHeadingSelector | ||
); | ||
const errors = await checkConsoleErrors(page, async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great but I think it has a chance to miss errors on certain element checks or with how the page element is being loaded. In this instance, errors could happen after the "Introduction" text has loaded which is what I'm guessing is happening in the following screenshot. There is a prop error not being caught on the DataManagement page:
I'm thinking the page.on('console')
listener could be done in getPage
and passed into the callback the same way the browser and baseUrl and checked in that way
@@ -0,0 +1,322 @@ | |||
import PropTypes from 'prop-types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, huge plus for putting this together and making all those changes. No more having to figure out how to account for certain structures :).
Also appreciate the care taken for identifying what should be required per prop type.
Thanks for the suggestion on how to handle the error tracking in e2e. It worked like a charm and is much cleaner! Interestingly it also uncovered an error that comes up on the reports page, "Failed to load resource: the server responded with a status of 404 (Not Found)". This only happens on CI. This likely existed previously but wasn't being checked. I can try to figure out what resource it is referring to tomorrow. |
I ended up adding a tag for a placeholder favicon so that the browser doesn't try to find a favicon by default. Used this solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super excited about the increased organization and deduplication something like this change brings. Thanks for putting this together and addressing my feedback!
@@ -5,6 +5,7 @@ | |||
<meta name="google" content="notranslate"> | |||
<meta name="viewport" content="width=device-width, initial-scale=1"> | |||
<link rel="stylesheet" href="/index.css"> | |||
<link rel="shortcut icon" href="data:image/x-icon;," type="image/x-icon" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting find!
This PR introduces reusable PropTypes. The proptypes are largely a mirror of the existing GraphQL query fragments. A few additional proptypes were added for situations with relatively complex
PropType.shape
s.To ensure that the PropTypes are honored and that no additional console errors are introduced by changes, I added a console error checker to the first load of pages during e2e testing.