-
Notifications
You must be signed in to change notification settings - Fork 394
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
Migrate codebase to Typescript and CSS Modules #1106
Conversation
fd7c836
to
38ebda9
Compare
38ebda9
to
3f85dc6
Compare
3f85dc6
to
79bd2c1
Compare
79bd2c1
to
5a8821a
Compare
d7b4617
to
bc25fec
Compare
bc25fec
to
b934a9f
Compare
…ssNames instead of global in few components
…Use it everywhere on main page. Refactor DownloadButton component
70a2cb1
to
5c6107b
Compare
Could you clarify it, point by point? In this case I can review it step by step too. I already fixed some of them, and some styles was changed after getting rid of duplication.
I’m not sure what do you mean when saying
Previous version was heavily dependent on interpolations from styles-components so I fully refactored it. Styles a little bit another, right. But here we can only improve them. What should I change?
Could you clarify what animation do you mean? |
"isomorphic-fetch": "^2.2.1", | ||
"lodash.fill": "^3.4.0", | ||
"lodash.includes": "^4.3.0", | ||
"lodash.kebabcase": "^4.1.1", | ||
"lodash.startcase": "^4.4.0", | ||
"lodash.throttle": "^4.1.1", | ||
"lodash.topairs": "^4.3.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.
aside: we have tree-shaking so we can simply do:
import { get } from 'lodash'
Knowing only get
is included in the bundle. So we don't have to install lodash bit by bit.
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.
Unfortunately not =( Tree shaking usually works like a 💩
There are a lot articles which describe this problem, like this one https://www.azavea.com/blog/2019/03/07/lessons-on-tree-shaking-lodash/
But we can replace it via import get from lodash/get
and it should work, but maybe it can be done in another PR (or tomorrow if it still won't be merged)
const logPR = (): void => logEvent('community', 'contribute-pr') | ||
const logBlogpost = (): void => logEvent('community', 'contribute-blogpost') | ||
const logTalk = (): void => logEvent('community', 'contribute-talk') | ||
const logAmbassador = (): void => logEvent('community', 'contribute-ambassador') |
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.
IIRC there's an option to not have to mark up every return type, if you get bored of doing that.
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.
Yep, but we intentionally enabled this rule to have more strict type checking
href="mailto:[email protected]?subject=I want to give a talk!" | ||
target="_blank" | ||
rel="noreferrer noopener" |
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.
Why remove this?
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 now automatically handled by Link component
|
||
// eslint-disable-next-line @typescript-eslint/interface-name-prefix | ||
interface Window { | ||
ga: (command: 'send', data: IGaEvent | IGaException) => void |
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.
Should be ga?
. Ad blockers and privacy extensions cause ga
to not load.
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.
window.ga
is set by gatsby-plugin-google-analytics
. I only described its interface
.eslintrc.json
after migration.tmp-empty-package
after getting rid ofstyled-components