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

player waiting scene 레이아웃/스타일링 #137

Merged
merged 6 commits into from
Dec 8, 2020

Conversation

ramram1048
Copy link
Collaborator

💁 설명

GIF 2020-12-07 오후 7-29-09

player waiting scene의 레이아웃, 스타일링을 작업했습니다.
모든 모듈에 독립적으로 작업되었기에 급하게 review하고 머지할 필요는 없어요.
socket event는 구현되있지 않습니다. 저녁 구현 및 통합 예정

GameObject에 move()와 거의 비슷한 roll() method를 구현 02870a0

  • 카드가 돌면서 떨어지는걸 만들기 위해 만들었습니다.
  • move() 거의 복붙이라서 나중에 겹치는 부분 refactor해야될 듯 합니다.
  • 사실 roll을 만들어야될지 rotate()를 js animate에서 requestAnimationFrame으로 refactor해서 move와 rotate를 동시에 썼어야할지 고민이 됐는데, 일단 그냥 빠르게 만들고싶어서 이렇게 만들었습니다.

z-index 변수 관리 문제

@scenes/playerWaiting/index.js 에서 setDepth()에 그냥 숫자를 때려박은 부분이 있는데 여기 들어가는 숫자들이 사실 @utils/common.scss 에 정의된 z-index값입니다.
이걸 어떻게 관리할지 모르겠네요...

webpack-contrib/sass-loader#170 (comment)
https://github.com/pmowrer/node-sass-json-importer
이런거 잘 사용하면 sass와 js에서 공통으로 사용할 variable을 json파일으로 관리할 수도 있을지도 모르는데 (시도는 안해봄) 의견이 듣고싶습니다.

헬프텍스트 가시성 문제

위 그림에서도 바로 보이지만 검은 글자가 오리, 카드와 겹쳐있으면 잘 안보이는데 흰색 배경이라도 넣어야될까요? 이것도 조언을 구합니다.

- move하면서 rotate하게하는 method. move와 달리 easing function이 적용되지 않았습니다.
- rollCount: 몇바퀴 돌건지. rollClockwise: 시계/반시계방향.
- TODO:
  - move()함수와 많은 부분 겹치므로 따로 뺄수있으면 빼자.
  - roll을 만들게 아니라 rotate를 roll처럼만들고 position과 독립적으로 동작하게 해야될까??
- duck, card가 포함되있지 않음
- event를 받을 때마다 duck을 생성하고 카드가 한장 떨어짐.
- z-index를 어떻게 관리할지가 걱정. scss에서도 쓰고 js에서도 쓴다.
@ramram1048 ramram1048 added this to the 4주차 milestone Dec 7, 2020
@ramram1048 ramram1048 self-assigned this Dec 7, 2020
Copy link
Collaborator

@Front-line-dev Front-line-dev left a comment

Choose a reason for hiding this comment

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

PR에 남겨주신 js와 sass의 변수관리는 알려주신 패키지로는 안 될거 같아요. 근간이 되는 node-sass가 deprecated 되었고 사용하기도 복잡한거같네요. 오늘 해람님이 고생하신거에 비해서 제가 너무 안 좋은말만 써놓아서 마음이 아프네요. 코드 리뷰는 그냥 제 생각이다 하고 봐주세요. 제 리뷰를 반드시 반영해야하는 이유는 없으니까요. 오늘 하루도 수고 많으셨습니다

@@ -147,6 +147,60 @@ const GameObject = class {
this.animationFrame = requestAnimationFrame(animateFunction);
}

roll(
Copy link
Collaborator

Choose a reason for hiding this comment

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

아예 roll 메소드를 구현하셨군요! move와는 다르게 중간에 끊길 일이 없어서 CSS로도 충분하지 않았나 생각해봅니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇네요 생각해보니...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Front-line-dev 시도해봤는데 transform rotateZ를 적용하는 과정에서 이유를 알 수 없게 의도치않은 동작이 나왔습니다. 예를들어 keyframe에 rotateZ(360deg)를 주면 카드가 1바퀴를 돌아야되는데 가만히 있는 동작을 보여주네요. transition을 이용하면 잘 되는데 animate()를 이용하면 안 되는게 저도 황당하네요. 일단은 보류하게 해주세요.

duck.setDepth(2);
duck.attachToRoot();
return duck;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

오늘 말씀해주신것처럼 오리는 waitingRoom에서 한번 만든 뒤, 안 사용하는 화면에서는 숨기는 식으로 하면 될거같아요

src/frontend/scenes/playerWaiting/style.scss Show resolved Hide resolved
Copy link
Collaborator

@sbyeol3 sbyeol3 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 rotate 어려우 ㅓ보이네요 ㅋㅋㅋ

return;
}
const newY = initialY + (targetY - initialY) * (elapsed / duration);
const newX = initialX + (targetX - initialX) * (elapsed / duration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

newY newX 로직이 같아서 이것도 별도 함수로 빼면 좋겠군용

src/frontend/engine/GameObject.js Outdated Show resolved Hide resolved

this.instance.style.left = makeUnitString(newX, '%');
this.instance.style.top = makeUnitString(newY, '%');
this.instance.style.transform = `rotateZ(${this.rotateStyle}) ${this.originStyle}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

y -> top / x -> left 이 관계가 있어서 이것도 뭔가 객체같은 걸로 빼면 편할 것 같아요!~


this.animationFrame = requestAnimationFrame(animateFunction);
}

rotate(angle = 0, duration = TIME.ONE_SECOND / 5) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIME.TWO_HUNDREDS_MILI_SEC 는 넘 길까요 ㅋ.ㅋ 흠

Copy link
Collaborator

Choose a reason for hiding this comment

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

애매하긴 하다..

Copy link
Member

Choose a reason for hiding this comment

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

음 개인적으로는 1초를 가장 작은 단위로 두고 이걸 사용하는게 오히려 util로써 의미를 가지지 않을까 싶기도 하네요! 위의 코드에도 argument 기본값이 TIME.ONE_SECOND / 2 로 설정되어 있는데 TIME.FIVE_HUNDREDS_MILI_SEC 이렇게 하나하나 다 나누면 오히려 이상할 것 같기도 합니다!

src/frontend/scenes/playerWaiting/style.scss Show resolved Hide resolved
@sbyeol3
Copy link
Collaborator

sbyeol3 commented Dec 7, 2020

해람님 이슈 안만들었죠 -____- ^^^ 🤟🤟

Copy link
Member

@jinhyukoo jinhyukoo left a comment

Choose a reason for hiding this comment

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

여윽시 CSS신 해람님! 멋진 작품 만드느라 고생하셨습니다!

this.instance.style.top = yString;
this.instance.style.left = xString;
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

오 이런 코드라면 위에서 하나씩 떨어지는거 말고 실제 게임처럼 각자 플레이어의 방향에서 카드가 날아오게 할 수도 있겠네요! 예를 들어 플레이어가 4명 있으면 상하좌우에서 하나씩 날아오는...? 갑자기 생각나서 이야기 해봤습니다! ㅋㅋ


this.animationFrame = requestAnimationFrame(animateFunction);
}

rotate(angle = 0, duration = TIME.ONE_SECOND / 5) {
Copy link
Member

Choose a reason for hiding this comment

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

음 개인적으로는 1초를 가장 작은 단위로 두고 이걸 사용하는게 오히려 util로써 의미를 가지지 않을까 싶기도 하네요! 위의 코드에도 argument 기본값이 TIME.ONE_SECOND / 2 로 설정되어 있는데 TIME.FIVE_HUNDREDS_MILI_SEC 이렇게 하나하나 다 나누면 오히려 이상할 것 같기도 합니다!

src/frontend/scenes/playerWaiting/style.scss Show resolved Hide resolved
- 기본값 magic number 제거
- roll 로직 "조금" 개선
- 다른 method들은 다 number를 받도록 되있는데 setOrigin만 '10%' 같은 string으로 받게 되어있었음
@ramram1048 ramram1048 merged commit f6dad7d into develop Dec 8, 2020
@ramram1048 ramram1048 deleted the feat/player-waiting branch December 8, 2020 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants