-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: cannot toggle custom toolbar state(active
, disabled
)
#1639
Conversation
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.
리뷰 완료합니다.
사소한거지만 classname 이어붙이기 할 때 상태가 엮여들어가면 역시 코드가 지저분해지게 되는데 classnames 같은 방식으로 정의하는 것은 어떻게 보시나요? 캘린더에도 이런 형태의 헬퍼 함수를 하나 만들어서 적용하려 합니다.
넵 좋네요ㅎㅎ 아래처럼 변경해서 조건에 따라 사용할게요~ export function cls(...names: (string | [boolean, string])[]) {
const result = [];
for (const name of names) {
let className: string | null;
if (Array.isArray(name)) {
className = name[0] ? name[1] : null;
} else {
className = name;
}
if (className) {
result.push(`${CLS_PREFIX}${className}`);
}
}
return result.join(' ');
} |
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.
리뷰완료합니다.
if (item.onUpdated) { | ||
if (prevProps.active !== active || prevProps.disabled !== disabled) { | ||
item.onUpdated({ active, disabled }); | ||
} | ||
} |
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.
if (item.onUpdated) { | |
if (prevProps.active !== active || prevProps.disabled !== disabled) { | |
item.onUpdated({ active, disabled }); | |
} | |
} | |
if (prevProps.active !== active || prevProps.disabled !== disabled) { | |
item.onUpdated?.({ active, disabled }); | |
} |
이렇게는 안될까요?
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.
좋네요ㅎㅎ
* fix: wron toolbarItems type definition * refactor: lift up changing toolbar state logic to hoc * chore: add test case(custom toolbar item active, disabled state) * chore: change toolbar-item-wrapper css name * chore: apply code review * chore: apply code review - 2
Please check if the PR fulfills these requirements
fix #xxx[,#xxx]
, where "xxx" is the issue number)Description
active
,disabled
)Thank you for your contribution to TOAST UI product. 🎉 😘 ✨