-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(sidebar navigation): update sidebar navigation styling #1649
base: main
Are you sure you want to change the base?
Conversation
@ingriddorioschulze can you check the possibility to add some unit tests? I'm aware the tests don't exist at the moment, but it would be cool to start to have some unit tests for non existing components 😃 |
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.
Great work! 👍 Could you add side-by-side before-screenshots for comparison? A bit hard to parse the css differences 😅
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 nice! 🎉
also +1 for the component testing 🙏
Quality Gate passedIssues Measures |
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.
Thanks for split the tests, they are much better now 😎
I would like to see every test verifying behavior independently of the child component. This way, we can decouple the tests, ensuring that changes to the child component don’t cause the parent component tests to break.
const nav = screen.getByRole("navigation"); | ||
expect(nav).toHaveClass("bg-white border-[1px] border-blue-400"); | ||
|
||
const list = screen.getByRole("list"); |
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.
if I change the NavigationList
to render a div without the role of list, this test will fail, but the error should be point in the NavigationList
test and not in the FlowNavigation
test.
My recommendation, we don't care here what the NavigationList
renders, only care what's is called inside of the FlowNavigation
. In the NavigationList.test.tsx
, we should test what the NavigationList
renders, so we test what the component does without expect what the child component does.
This PR contains the new sidebar navigation styling:
Before
After