-
Notifications
You must be signed in to change notification settings - Fork 3
Resume ( Timeline Component ) #17
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
base: develop
Are you sure you want to change the base?
Conversation
|
@tolicodes Here's the chromatic link: https://www.chromatic.com/build?appId=61e090acefc5bd003a506777&number=5 |
tolicodes
left a comment
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.
good job! Let's merge and pair program next week on refactoring.
| test("Carousel", () => { | ||
| expect(() => { | ||
| render(<Carousel />); | ||
| }).not.toThrowError(); |
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.
Add a snapshot test
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 have added content based test, but using testing-library, it is a little hard to do snapshot test, because the class name is randomly generated when style-components are being used, because I'm unaware of what they would be. I believe storybook-testing might be better and might provide us with snapshot testing for our scenario, Please advice.
| interface TimelineProps { | ||
| timeline: ITimeline; | ||
| } | ||
| const Timeline: React.FC<TimelineProps> = ({ timeline }) => { |
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.
We can merge for now, but let's pair program next week on optimizing this. This is a very important component
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.
Sure
| const Resume = ({}: ResumeProps) => { | ||
| return ( | ||
| <SectionWrapper title="Resume"> | ||
| {/* Carousel for companies */} |
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.
Why commented?
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 began by laying the base for carousel but shifted to Timeline, because that had higher priority. I will be coming back to this one.
|
Images don’t work. Also make sure it looks good on mobile.
…On Wed, Jan 19, 2022 at 3:10 AM thehexatown-zaryab ***@***.***> wrote:
@tolicodes <https://github.com/tolicodes> Here's the chromatic link:
https://www.chromatic.com/build?appId=61e090acefc5bd003a506777&number=5
—
Reply to this email directly, view it on GitHub
<#17 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHFP6ZQ3QJTCDIFZAZSNQKDUWZWYRANCNFSM5MIGF7MA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
No description provided.