Conversation
percypie
commented
Nov 24, 2021
percypie
commented
ralph-dev
left a comment
ralph-dev
left a comment
There was a problem hiding this comment.
Not approving since todo let
|
|
||
| `; | ||
|
|
||
| //todo Make location change with amount of profiles (currently having issues trying to make Featured profile card not generate a new line without being able to edit it) No newline at end of file |
ralph-dev
left a comment
ralph-dev
left a comment
There was a problem hiding this comment.
Why not update the original component to have this feature?
|
I assumed I wasn't allowed to edit the existing component, if I can edit the existing component then I can fix this very easily |
ralph-dev
left a comment
ralph-dev
left a comment
There was a problem hiding this comment.
What if the user doesn't want this. When you edit the existing component you need to make sure not to break existing users workflows ;)
| {renderProfileCircles()} | ||
| <FeaturedProfile icon key={ADD_USER_ICON_KEY} background="grey" /> | ||
| </Container> | ||
| <CountContainer>{profileData.length}</CountContainer> |
There was a problem hiding this comment.
This needs to be optional
Prop && Jsx
|
|
||
| export interface IFeaturedProfilesCardProps { | ||
| profileData: IProfile[]; | ||
| counterBool: boolean; |
There was a problem hiding this comment.
Documentation of Props goes here not on the component. You are doucmenting the type
| export const FeaturedProfilesCard: React.FC<IFeaturedProfilesCardProps> = ({ | ||
| profileData, | ||
| profileData, //the array of profiles to be displayed | ||
| counterBool, //if true, the profile counter is displayed, and if false, the counter is not displayed |
There was a problem hiding this comment.
docs should not be on this line, put in TypeScript
ralph-dev
left a comment
ralph-dev
left a comment
There was a problem hiding this comment.
Fix the Comments before merging, LGTM
| export const Basic = Template.bind({}); | ||
| Basic.args = { | ||
| profileData: profiles, | ||
| counterBool: false, |
There was a problem hiding this comment.
Don't put Types in the Name
We dont say NameString, we just say Name.
Rename this to isCounterShowing or isCounterDisplayed or isProfileCountShowing
the prefix is denotes a Boolean value
ralph-dev
left a comment
ralph-dev
left a comment
There was a problem hiding this comment.
LGTM, I would update variable name
2 participants
