-
Notifications
You must be signed in to change notification settings - Fork 30
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
Created RoleCard Component #1129
Created RoleCard Component #1129
Conversation
|
import React from 'react' | ||
import { RoleCardContainer, PicWrapper, TextContents, IconImg} from './StyledRoleCard' | ||
|
||
// @description This component renders the RoleCard 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.
you can comment in multiple lines using /** @description ....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.
Completed.
return ( | ||
<RoleCardContainer> | ||
|
||
<PicWrapper> |
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.
Rename this styled component to 'IconWrapper', this explains the code better
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.
Completed.
|
||
export const RoleCardContainer = styled.div` | ||
margin-top: 50px; | ||
/* 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.
Not 100% sure if this is the right type of comments for this, but if your not receiving any errors, then this is fine. could you double check in the console: so when you run the app, inspect the code through the browser, then head to the console, any errors printed? if not this is fine
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.
There was no error.
switch (props.theme) { | ||
case 'theme1': | ||
return ( | ||
<TextWrapper> |
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 works fine., but is there a way to refine this condition to be cleaner? so we're not duplicating the <Typography style={....}/> .... <Typography />
x2 for each case. Just an idea - this logic does not have to remain in the StyledRoleCard.js file, have a look at adding classes dynamically to html tags e.g <TextWrapper className={'${theme}'}
etc
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.
Added classes for each theme dynamically.
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.
Can you revert your changes. There's no need to push this into development/user-profile branch. As the role card is not meant to be shown here. This should only be in your local/task branch.
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.
Deleted the RoleCard Component from UserOnboardingModal.js.
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've reviewed your work. Can you revert the UserOnboardingModal.js back to the default version thats synced with development/user-profile branch. Once done let me know.
<RoleCard iconImg={"Onboarding"} title={"Developer"} subtitle={"You are here to work on our codebase"} theme={"theme1"} />