-
Notifications
You must be signed in to change notification settings - Fork 646
chore: add className testing util #7231
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
base: main
Are you sure you want to change the base?
Conversation
|
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
| 'prc-Overlay-Overlay-ViJgm', | ||
| component => component.container.firstChild!.childNodes[1].firstChild?.firstChild as HTMLElement, | ||
| props => <AnchoredOverlayTestComponent initiallyOpen={true} {...props} />, | ||
| ) |
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.
my dream test function call looks like this:
implementsClassName(ActionList)
// or if we need to,
implementsClassName(props => <AnchoredOverlayFixture {...props} />)implementsClassName should
- render without custom className to figure out the base class and base element
- render with custom className, make sure it has both classes on the base element
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.
render without custom className to figure out the base class and base element
can you elaborate on this one? I'm confused on how it would do that 🤔
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.
(i'm literally making this up so it might not work, i should invest some time doing a proof of concept) Is there a pattern we follow for base classes? Does it always have the component name like in prc-Overlay-Overlay-ViJgm, is it safe to find the first prc-* class we find, that's probably the base class?
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.
yeah we could, the main problem I see is we don't have (as far as I can tell) "getByClassName" or something of the sorts util, so even knowing the full className, I don't know how to find the element is my main problem.
Another potential roadblock I see is some components are composed of others so will definitely have multiple "prc-*" classes
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.
Update: yes we can look for the className, cleaned it up to where we're just passing the render fixture and the className, I'm not sure about trying to infer the className just by looking for prc-* because of what I said ^ regarding composed components, let me know what you think!
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.
(thinking out loud, non blocking, ignore for now)
so even knowing the full className, I don't know how to find the element is my main problem
Is it possible to run query selectors like document.querySelector('[class^=prc]') (class starts with prc) and the first result should be the element with base class?
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.
Is it possible to run query selectors like document.querySelector('[class^=prc]') (class starts with prc) and the first result should be the element with base class?
Here's an example where this doesn't prove true:
The ActionMenu is wrapped within an Overlay, so the first PRC class is not the one we're looking for here
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.
Ohhh, righto! I don't know why I understand this earlier.
so even knowing the full className, I don't know how to find the element is my main problem
Is this still valid?
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.
Is this still valid?
No, I figured how to find classNames out, but I still think we need to supply the className we're looking for as a parameter due to #7231 (comment) 👀
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
Closes https://github.com/github/primer/issues/5255
This pull request adds a reusable test utility to verify that components correctly handle
classNameprops and applies it to the components' tests. The main focus is on improving test coverage and consistency for CSS class name behavior.Changelog
New
implementsClassNamefunction toutils/testing.tsx, which provides a standardized way to test that components render both their default and customclassNameprops.implementsClassNameinto existing components test suite, ensuring that the components correctly apply both the base and custom class names.Rollout strategy
Test-only update
Testing & Reviewing
Merge checklist