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

fix!: FloatAreaのresponseMessage属性の型を実装に合わせて調整する #5299

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AtsushiM
Copy link
Member

関連URL

概要

変更内容

確認方法

Copy link

pkg-pr-new bot commented Jan 17, 2025

Open in Stackblitz

npm i https://pkg.pr.new/kufu/smarthr-ui@5299

commit: 02985e3

}),
[style, zIndex],
)
const styleAttr = useMemo(() => ({ ...style, zIndex }), [style, zIndex])
Copy link
Member Author

Choose a reason for hiding this comment

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

object化して再展開していますが、内部に属性がstyleのみだったため、あまり意味がありませんでした。
style={hoge} 形式で指定させることにして調整しました

Copy link
Collaborator

Choose a reason for hiding this comment

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

zIndex は効いてなかった?

Copy link
Member Author

@AtsushiM AtsushiM Jan 20, 2025

Choose a reason for hiding this comment

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

↑、あそういう意味ではなくて、設定する属性はstyle属性のみで { style: hoge } のようなオブジェクトでmemo化する意味が薄い、という話でした。
もともとの処理としては {...styleProps} の ような方法で

  • 中身のキーは一つなので混乱するだけで意味が薄い
  • オブジェクトの展開分コストが無駄

という感じです。

Copy link
Member Author

Choose a reason for hiding this comment

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

>
<Cluster gap={1}>
{tertiaryButton}
<div className="shr-ms-auto">
<Cluster gap={1} align="center">
{(responseMessage?.status === 'success' || responseMessage?.status === 'error') && (
Copy link
Member Author

Choose a reason for hiding this comment

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

statusがsuccess, errorのもののみに絞っていますが

  • processingなresponseMessageが表示されない
  • 理由もわからない

という感じなのでそもそも型として受け取れないように調整しました
これにより、チェックロジックもシンプルに変更できます

@AtsushiM AtsushiM changed the title chore: FloatAreaの内部ロジックを整理する fix!: FloatAreaのresponseMessage属性の型を実装に合わせて調整する Jan 19, 2025
@AtsushiM AtsushiM marked this pull request as ready for review January 19, 2025 22:45
@AtsushiM AtsushiM requested a review from a team as a code owner January 19, 2025 22:45
@AtsushiM AtsushiM requested review from misako0927 and masa0527 and removed request for a team January 19, 2025 22:45
}),
[style, zIndex],
)
const styleAttr = useMemo(() => ({ ...style, zIndex }), [style, zIndex])
Copy link
Collaborator

Choose a reason for hiding this comment

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

zIndex は効いてなかった?

packages/smarthr-ui/src/components/FloatArea/FloatArea.tsx Outdated Show resolved Hide resolved
@AtsushiM AtsushiM requested a review from uknmr January 20, 2025 00:03
@AtsushiM AtsushiM marked this pull request as draft January 21, 2025 02:49
@AtsushiM AtsushiM marked this pull request as ready for review January 21, 2025 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants