Fixed errors on routes [/, /courses, /events, /campusLeaders]#99
Fixed errors on routes [/, /courses, /events, /campusLeaders]#99aymenhamada wants to merge 2 commits intocodeforcauseorg:developmentfrom
Conversation
|
@Abhishek-kumar09 Can yo do detailed analysis of this one? And provide insights. |
GangaChatrvedi
requested review from
Abhishek-kumar09 and
GangaChatrvedi
and removed request for
Abhishek-kumar09
Abhishek-kumar09
left a comment
Abhishek-kumar09
left a comment
There was a problem hiding this comment.
Here are descriptions and some questions asked on changes, But we have to additionally cross-check and compare the original and this deployed version twice before actually merging the PR.
Thanks @aymenhamada for your efforts.
| <Box ml={2} flexGrow={1} /> | ||
| {navItems.map((item, index) => ( | ||
| <Item | ||
| key={index} |
There was a problem hiding this comment.
- Each child in a list should have a unique "key" prop.
| return <MenuItem value={`+${code}`}>+{code}</MenuItem>; | ||
| {countryCodes.map((code, index) => { | ||
| return ( | ||
| <MenuItem key={index} value={`+${code}`}> |
There was a problem hiding this comment.
- Each child in a list should have a unique "key" prop.
| align="center" | ||
| display="flex" | ||
| justifyContent="center" | ||
| alignItems="center" |
There was a problem hiding this comment.
- AlignItems must be used on containers.
| xs={12} | ||
| align="center" | ||
| display="flex" | ||
| justifyContent="center" |
There was a problem hiding this comment.
- AlignItems must be used on containers.
| align="center" | ||
| display="flex" | ||
| justifyContent="center" | ||
| alignItems="center" |
src/views/pages/CLView/StandFor.js
Outdated
| contribute for the community<h4>- An Initiative taker</h4> | ||
| contribute for the community | ||
| </Typography> | ||
| <Typography variant="h4">- An Initiative taker</Typography> |
There was a problem hiding this comment.
❌ The text looks actually bigger than the original one.
There was a problem hiding this comment.
Try changin to h5 or h6 to better comparable to the original design.
src/views/pages/CLView/StandFor.js
Outdated
| helpful community <h4>- A Resource Seeker</h4> | ||
| helpful community | ||
| </Typography> | ||
| <Typography variant="h4">- A Resource Seeker</Typography> |
There was a problem hiding this comment.
❌ Here also, The bold text looks actually bigger than the original one.
| <Box mt={2}> | ||
| <Typography variant="body1"> | ||
| Most sought after value - an effort maker and giving | ||
| peronality <h4>- An Investor</h4> |
| <Grid | ||
| item | ||
| key={course.id} | ||
| key={cname} |
There was a problem hiding this comment.
cname, not course.id
There was a problem hiding this comment.
course.id was undefined, and cname was unique
| }} | ||
| {...rest} | ||
| > | ||
| <Grid container maxWidth="lg"> |
There was a problem hiding this comment.
React does not recognize the
maxWidthprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasemaxwidthinstead. If you accidentally passed it from a parent component, remove it from the DOM element.
I think it was intentionally put here, so please change maxWidth to maxwidth
|
|
|
The bold letters should be small, see sandbox.codeforcause.org |
|
This should be one shade bold, see sandbox.codeforcause |
|
please rebase to resolve conflicts. |
|
idk how to fix that issue, help needed |
Which issue are you talking about, rebasing? |
|
y |
|
Steps you have to do:
|
3 participants
issue: #92