[material-ui][Divider] Enable borderStyle enhancement in divider with children #42715
Conversation
anuujj
changed the title
Netlify deploy previewhttps://deploy-preview-42715--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
DiegoAndai
left a comment
DiegoAndai
left a comment
There was a problem hiding this comment.
Thanks for working on this @anuujj! I left a question.
The solution is working as expected: https://codesandbox.io/p/sandbox/pr-42715-1-pyqg9y
DiegoAndai
left a comment
DiegoAndai
left a comment
There was a problem hiding this comment.
May I ask you to add a test to prevent future regressions? 😊
|
Hi @DiegoAndai, I tried adding a test for this but ran into a limitation of JSDOM i.e it does not implement the cascading of styles. So, the styling in test is different from the behaviour in browser. Similar issue is highlighted here #24761, if you are aware of any workaround, feel free to suggest that. |
ZeeshanTamboli
added
package: material-ui
type: enhancement
There was a problem hiding this comment.
Hi @DiegoAndai, I tried adding a test for this but ran into a limitation of JSDOM i.e it does not implement the cascading of styles. So, the styling in test is different from the behaviour in browser. Similar issue is highlighted here #24761, if you are aware of any workaround, feel free to suggest that.
Can you try to skip the test in JSDOM as suggested in the warning above:
if (/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}You can take a look at other test files on how it's done. When you skip the test in JSDOM, it will run on browser instead.
| } | ||
| }); | ||
|
|
||
| it('should set the border-style dashed in before and after pseudoclass', () => { |
There was a problem hiding this comment.
@anuujj here's an example on how to assert:
We use expect.
On another note, the border left style should only be applied with the vertical prop, so we'll need two tests. One for horizontal and one for vertical.
There was a problem hiding this comment.
yeah, I was low on bandwidth this week and having some issue with the local test setup. I will try to fix these on the weekend. Moving the PR to draft for now.
There was a problem hiding this comment.
added two tests as you suggested and used expect for assertions. You can review now. Thanks.
anuujj
force-pushed
the
dashed-border-divider
branch
from
ede8015 to
2fd81b7
Compare
|
@anuujj I pushed a commit to fix the test structure and description in 89dcd02. Rest looks good. @DiegoAndai to review. I think we can cherry-pick this to master. |
ZeeshanTamboli
changed the title
ZeeshanTamboli
added
the
needs cherry-pick
ZeeshanTamboli
changed the title
DiegoAndai
left a comment
DiegoAndai
left a comment
There was a problem hiding this comment.
LGTM, thanks @anuujj and @ZeeshanTamboli.
5 participants
Closes #42569