-
Notifications
You must be signed in to change notification settings - Fork 145
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
Fix unit test interdependence #84
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.
Lgtm! Thanks for tackling this
vizro-core/changelog.d/20231001_000340_huong_li_nguyen_fix_test_interdependence.md
Show resolved
Hide resolved
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 really like the refactored code. It's so easy to read and understand now.
cb8b448
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 like the new idea of moving it to the validator, this improved the code readibility a lot! And we can remove the registry access in one place only if we ever want to
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 shared a little note here, but feel free to overlook it and go ahead with the merge if it doesn't quite resonate with you 😄.
Description
test_page_build_left_side_removed
(could previously not be added due to interdependence)Accordion
model (as part of an investigation on whether we can get rid of the dashboard.build dependence)Screenshot
Checklist
Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1))
(if applicable)Types of changes
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":