trander123 / My-Portfolio

0 stars 0 forks source link

Comments #1

Closed ezerssss closed 1 year ago

ezerssss commented 1 year ago

Firstly ang wrong nimo kay wala ka nag Typescript, jkjk HAHAHA pero recommend jud nako typescript for js projects bai hahah

Self-close tags https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/Router.js#L11-L14

Remove flex and flex-col since that is the default behaviour of a block div https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Navbar.jsx#L8

Put this div in the NavAvatar component, we don't really care how NavAvatar is contained or whatever, we just abstract it. I noticed you just used a fragment in the NavAvatar component so just replace that with this div. https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Navbar.jsx#L9

Also here no need actually for flex and flex-col but since ur children are not block elements it kinda works here. https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Navbar.jsx#L13

Replace with the actual div parent. https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/NavAvatar.jsx#L5

Remove the flex and flex-col and use block elements like <p> instead of <span> https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/NavAvatar.jsx#L7-L12

ezerssss commented 1 year ago

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Header.jsx#L5-L20

For this bai daghan kog mga comments:

Nice na imo naming sa imo states kay naay "is" nga prefix nya sakto kay boolean pero ang imo function names kay based sa ako nabawan, mas cleaner if i rename nimo to handleToggleTheme ug handleToggleNavBar, pero preference raman sad na siya

Ang pinaka important para nako kay ang gihabuhat sa imo functions, dili siya "React like", there's a better way to do this bai, first kay

So if ani ang implementation kay cleaner kay ato states kay ga do jud sa ila jobs nya ang ato mga rendering components kay ila concern rajud is ang pag display sa mga shit. Goods!

ezerssss commented 1 year ago

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Home.jsx#L7-L14

Ma remove ra nimo ang kanang div bai, kay imo outer parent kay ga flex na nya ga justify-center ug items-center goods nana

ezerssss commented 1 year ago

Name the variables properly bai, better guro nga datas.map((project, index) => )

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L9

di nalang ko mo check pero basically feel nako ga overuse kag flex flex-col hahahahaha

Proper variable name technologies.map((technology, index) =>, AND wala sad nimo na butngan ug key ang imo map, hmm para ma unique ang key i think template literal sa ${project.projName}, ${index} para unique jud? https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L16-L18

Hmm pwede pud guro nga instead nga datas ang folder name kay better guro constants? debatable pero mao na ako i name sa folder if ako hahaha

trander123 commented 1 year ago

Name the variables properly bai, better guro nga datas.map((project, index) => )

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L9

di nalang ko mo check pero basically feel nako ga overuse kag flex flex-col hahahahaha

Proper variable name technologies.map((technology, index) =>, AND wala sad nimo na butngan ug key ang imo map, hmm para ma unique ang key i think template literal sa ${project.projName}, ${index} para unique jud?

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L16-L18

Hmm pwede pud guro nga instead nga datas ang folder name kay better guro constants? debatable pero mao na ako i name sa folder if ako hahaha

Name the variables properly bai, better guro nga datas.map((project, index) => )

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L9

di nalang ko mo check pero basically feel nako ga overuse kag flex flex-col hahahahaha

Proper variable name technologies.map((technology, index) =>, AND wala sad nimo na butngan ug key ang imo map, hmm para ma unique ang key i think template literal sa ${project.projName}, ${index} para unique jud?

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L16-L18

Hmm pwede pud guro nga instead nga datas ang folder name kay better guro constants? debatable pero mao na ako i name sa folder if ako hahaha

Kayasa hawda Ezra oi

trander123 commented 1 year ago

Firstly ang wrong nimo kay wala ka nag Typescript, jkjk HAHAHA pero recommend jud nako typescript for js projects bai hahah

Self-close tags

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/Router.js#L11-L14

Remove flex and flex-col since that is the default behaviour of a block div

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Navbar.jsx#L8

Put this div in the NavAvatar component, we don't really care how NavAvatar is contained or whatever, we just abstract it. I noticed you just used a fragment in the NavAvatar component so just replace that with this div.

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Navbar.jsx#L9

Also here no need actually for flex and flex-col but since ur children are not block elements it kinda works here.

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/Navbar.jsx#L13

Replace with the actual div parent.

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/NavAvatar.jsx#L5

Remove the flex and flex-col and use block elements like <p> instead of <span>

https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/components/NavAvatar.jsx#L7-L12

Hahaha medyo hasulan kos Typescript kay more on showing some shit rani HAHAHA wala kaayo ka involve ug data strict shit HAHAHAHA

trander123 commented 1 year ago

Name the variables properly bai, better guro nga datas.map((project, index) => ) https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L9

di nalang ko mo check pero basically feel nako ga overuse kag flex flex-col hahahahaha Proper variable name technologies.map((technology, index) =>, AND wala sad nimo na butngan ug key ang imo map, hmm para ma unique ang key i think template literal sa ${project.projName}, ${index} para unique jud? https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L16-L18

Hmm pwede pud guro nga instead nga datas ang folder name kay better guro constants? debatable pero mao na ako i name sa folder if ako hahaha

Name the variables properly bai, better guro nga datas.map((project, index) => ) https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L9

di nalang ko mo check pero basically feel nako ga overuse kag flex flex-col hahahahaha Proper variable name technologies.map((technology, index) =>, AND wala sad nimo na butngan ug key ang imo map, hmm para ma unique ang key i think template literal sa ${project.projName}, ${index} para unique jud? https://github.com/trander123/My-Portfolio/blob/436f0a0a61e3de90782f521994dc7314e5bb0aa4/src/pages/Portfolio.jsx#L16-L18

Hmm pwede pud guro nga instead nga datas ang folder name kay better guro constants? debatable pero mao na ako i name sa folder if ako hahaha

Kayasa hawda Ezra oi

Ako na eh apply bai