-
Notifications
You must be signed in to change notification settings - Fork 26
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
Toggle graphs in the training board olena #825
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.
thanks for the work! a few comments on how to improve it, please also fix the tests/lint found by the CI
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! The TrainingInformation's test can still be improved a bit :)
Can you resolve tharvik's comments if addressed them?
}, | ||
}); | ||
|
||
expect(wrapper.findAll("#mapHeader li")).toHaveLength(3); | ||
const listItems = wrapper.findAll("#mapHeader li"); | ||
expect(listItems.length === 0 || listItems.length === 3).toBe(true); |
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.
Only one length should be expected (is the advanced information open or closed by default?).
Can you break this down into two assertions: one when the length is expected to be 0 and another when the length should be 3? You can use cypress' .click() to toggle the advanced information.
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.
This test is designed to validate the Training Logs section, which has been removed. Therefore, the test should only pass when the length is 0. However, does it make sense to keep this test at all, considering there is essentially nothing left for it to verify?
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.
Indeed you can remove it altogether!
Hey @iamsherlocked1891, could you wait until Dec 5th to merge this PR? There a DISCO demo happening tomorrow so it would be safer to wait until it's past :) |
3ec8015
to
b807d92
Compare
Client-side
Additions & Modifications:
Enhanced DropdownCard Component:
Refinements in the Training Information Section:
Removed Training Logs from Training Information: