Table Component Rounded Corners Fix #3010
Conversation
|
|
||
| const wrapperDiv = screen.getByRole('table').closest('div'); | ||
|
|
||
| expect(wrapperDiv).toHaveClass(/rounded/, 'overflow-hidden'); |
There was a problem hiding this comment.
Generally, it's not the best idea to test for the presence of static CSS classes - the overflow in this case -, since changing CSS would imply changing a lot of test cases as well.
If the class is dynamic, it's okay, but I prefer not to, because it makes changing CSS cumbersome. Furthermore, does the presence of a CSS class not really tell the whole story of if the CSS acts according to the expectations.
There was a problem hiding this comment.
@janvhs, so I'm not too sure how to propose a change in the test here 😅... the condition that the test is checking for is the existence of both the rounded* and overflow-hidden classes which are required for the rounded corner effect. I could just check for the overflow*class but it won't yield the same result in the UI unless it is specially the overflow-hidden class. Please let me know if this what you were referring to.
There was a problem hiding this comment.
No, wat I wanted to get at is, that you usually don't need a test case for something like this, because you didn't touch "actual" logic ^^
There was a problem hiding this comment.
Ah yes, I agree. I did raise it in the Test Section that the test might be too much work for such a little change. But at the same time I felt like it just helpful to try use a test to try safeguard a Design System aesthetic. 😅
There was a problem hiding this comment.
Yeah but you'd be better of safeguarding it via some kind of visual test... be it done via screenshots and regression testing or manually via storybook, like we do. If one wants to automate this, there are tools for spotting diffs like https://github.com/dmtrKovalenko/odiff or our openQA. Otherwise you try to break down the cascading nature of CSS to isolated units, which just doesn't work reliably
There was a problem hiding this comment.
Yeah we also have visual tests in our pipeline because Chromatic does them, I just disabled them because they cost additional money and due to the nature of our stories using fixtures the always changing data was conflicting with snapshot tests.
Feel free to merge, @jagabomb!
There was a problem hiding this comment.
@janvhs thanks for the suggestion... I will definitely look into it for next time.
Description
This PR attempts to fix a visual issue where tables were not displaying the rounded corners correctly even though that had the correct corner rounding classes. An
overflow-hiddenclass was added since nested elements (th and td items) have background colours.Changes (Before and After)
How was this tested?
Describe the tests that have been added/changed for this new behaviour.
roundedclass andoverflow-hiddenclass. (Might be overkill but I am open to feedback)