Skip to content
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

#108: Sidebar/108 #120

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

#108: Sidebar/108 #120

wants to merge 5 commits into from

Conversation

cychann
Copy link
Member

@cychann cychann commented Mar 25, 2022

작업 내용

  • article 페이지에 sidebar 추가했습니다.

스크린샷 2022-03-25 오전 11 51 38

@cychann cychann added 💡 Feature New feature or request 👀 In review 리뷰 요청중인 상태 labels Mar 25, 2022
@cychann cychann self-assigned this Mar 25, 2022
@netlify
Copy link

netlify bot commented Mar 25, 2022

Deploy Preview for todaytheylearned ready!

Name Link
🔨 Latest commit 17e4ed6
🔍 Latest deploy log https://app.netlify.com/sites/todaytheylearned/deploys/62453e46b19e1b0009052230
😎 Deploy Preview https://deploy-preview-120--todaytheylearned.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cychann cychann changed the title Sidebar/108 #108: Sidebar/108 Mar 25, 2022
Comment on lines 28 to 25
const setArr = [];
const [tagArr, setTagArr] = useState([]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tags 어떠십니까

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영했습니다 👍

Comment on lines -29 to +31
{location.pathname === '/' ? <SideBar /> : null}
{location.pathname === '/' || location.pathname.slice(0, 8) === '/article' ? (
<SideBar />
) : null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이구조 한번 다같이 리팩토링하져

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그래야 할 것 같아유

soyekwon
soyekwon previously approved these changes Mar 27, 2022
Copy link
Contributor

@soyekwon soyekwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 잘 봤습니당

minjj0905
minjj0905 previously approved these changes Mar 28, 2022
@cychann cychann dismissed stale reviews from minjj0905 and soyekwon via 4d48a26 March 29, 2022 02:50
@cychann cychann added 👏 Merge Needed 해당 pr이 approve되어 merge 가능한 경우 and removed 👀 In review 리뷰 요청중인 상태 labels Mar 29, 2022
@cychann cychann marked this pull request as draft March 29, 2022 02:54
@cychann cychann added 👀 In review 리뷰 요청중인 상태 and removed 👏 Merge Needed 해당 pr이 approve되어 merge 가능한 경우 labels Mar 29, 2022
@cychann cychann marked this pull request as ready for review March 29, 2022 02:56
Comment on lines 28 to 26
const setArr = [];
const [tagsArr, setTagsArr] = useState([]);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p5: tags, setTags 로 하자는거였는뎀 ㅠ

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그냥 봐도 array인데 arr을 명시하는건 너무 이상함다

Copy link
Member Author

@cychann cychann Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아앗 그냥 tags.. 다시 커밋했습니당

Copy link
Member

@shinkeonkim shinkeonkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Feature New feature or request 👀 In review 리뷰 요청중인 상태
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants