-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(www): localize new contact view #1735
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.
just some nits/thoughts
src/www/.storybook/localize.ts
Outdated
@@ -14,17 +14,11 @@ | |||
limitations under the License. | |||
*/ | |||
|
|||
import {FormattableMessage, LocalizeFunc} from 'src/infrastructure/i18n'; |
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.
you can use import type
!
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.
Done, though I've never fully understood why this is better when importing TS code, since the compiler automatically identifies imports that are only used as types. I thought in this particular case it has no real effect?
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.
It's just explicit to the developer IMO. And there are so MANY ways to compile in JS (tsc, babel, swc, esbuild) it's hard to guarantee what a given compiler is doing
b5d3c2a
to
aa6f61d
Compare
18c0b17
to
8f14ff4
Compare
aa6f61d
to
ecd985a
Compare
FYI moved the |
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
d420b8c
to
ca4522e
Compare
No description provided.