-
Notifications
You must be signed in to change notification settings - Fork 141
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
chore: AppHeader/mobile の内部処理を整理する vol.1 #5392
base: master
Are you sure you want to change the base?
Conversation
…e-refactoring-AppHeader
…e-refactoring-AppHeader
…e-refactoring-AppHeader
1196c3d
to
6a186de
Compare
6a186de
to
032a8d2
Compare
commit: |
…e-refactoring-AppHeader-mobile
}) | ||
} | ||
|
||
const looseSearchQuery = normalize(searchQuery) |
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.
normalizeは1文字ずつ変換処理を行うため、かなり重い処理になります。
元々のロジックではlooseIncludeで毎回変換が行われていたため事前に変換処理を行います
if (firstName && lastName) { | ||
return `${lastName} ${firstName}${empCode ? `(${empCode})` : ''}` | ||
} | ||
|
||
return ( | ||
(firstName && lastName ? `${lastName} ${firstName}` + empCodeStr : empCode ? empCode : email) ?? | ||
'' | ||
) | ||
return empCode || email || '' |
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.
元のロジックでは不必要なempCodeStrが生成される場合があったため、条件分岐で整理しています
…e-refactoring-AppHeader-mobile
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.
良さそう!1点だけ質問のコメントしてます!
const Header = isDesktop ? DesktopHeader : MobileHeader | ||
|
||
return ( | ||
<LocaleContextProvider locale={locale}> | ||
<DesktopHeader {...props}>{isDesktop && children}</DesktopHeader> | ||
<MobileHeader {...props}>{isMobile && children}</MobileHeader> | ||
<Header {...props}>{children}</Header> |
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.
[ask] この変更で上記コメントはいらなくなりますかね?
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.
上記 is
smarthr-ui/packages/smarthr-ui/src/components/AppHeader/AppHeader.tsx
Lines 12 to 14 in b5b23f3
// NOTE: ヘッダーの出し分けは CSS によって行われているので、useMediaQuery による children の出し分けは本来不要ですが、 | |
// wovn の言語切替カスタム UI の挿入対象となる DOM ("wovn-embedded-widget-anchor" クラスを持った div) が複数描画されていると、 | |
// wovn のスクリプトの仕様上1つ目の DOM にしか UI が挿入されないため、やむを得ず children のみ React のレンダリングレベルでの出し分けをしています。 |
関連URL
概要
変更内容
確認方法