-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: circular selection with keyboard (#2), #6
Conversation
src/js/dropdown.js
Outdated
index += direction + length; | ||
index %= length; | ||
|
||
return index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (index + direction + length) % length;
한줄로 줄일 수 있을 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
완료입니다
src/js/dropdown.js
Outdated
for (; index < items.length && index >= 0; index += direction) { | ||
index = this.getItemIndex(index, length, direction); | ||
|
||
while (index < length && index >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index >= 0 조건이 필요하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰완료합니다.
// 'IE11', | ||
// 'Edge', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석처리한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트가 정상 진행되지 않아 주석처리 했습니다.
IE11의 경우 테스트를 수행하고자 하는 흔적(브라우저가 켜져 있는 등)이 있으나 Error response status: 13, UnknownError - An unknown server-side error occurred while processing the command.
를 출력하며 테스트가 정상 진행되지 않았습니다.
따라서 주석 처리 후 IE11 상에서 동작 테스트를 대신 진행했습니다.
* @param {number} direction - direction to move | ||
* @return {number} | ||
*/ | ||
getItemIndex(index, length, direction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public API로 나가는거에요? 그럴만한 메서드가 아닌 것 같아서요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSDoc 삭제해서 올리겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 완료입니다!
chore: remove unnecessary contidion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰완료합니다.
Please check if the PR fulfills these requirements
fix #xxx[,#xxx]
, where "xxx" is the issue number)Description
as-is
to-be
Thank you for your contribution to TOAST UI product. 🎉 😘 ✨