-
-
Notifications
You must be signed in to change notification settings - Fork 773
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
Add eslint with eslint-config-airbnb #748
Conversation
(This is also required for Vite later)
These are safe fixes, more complicated fixes can be done separately (just disabled those errors for now). - Reorder declarations to fix `no-use-before-define` - Rename parameters for `no-shadow` - Remove unused parameters, functions, imports - Switch from `++` and `—` to `+= 1` and `-= 1` for `no-unary` - Use object spreading instead of parameter reassignment in auth utils - Use `window.location` instead of `location` global - Use inline JSX strings instead of unescaped values -
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.
Still not done. But here are more comments.
@@ -30,11 +30,14 @@ export const AccountContext = createContext(null); | |||
|
|||
const App = () => { | |||
const [account, setAccount] = useState(null); | |||
|
|||
const contextValue = useMemo(() => ({ account, setAccount }), [account, setAccount]); |
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.
Explain this? Sorry, my web-fu is not strong.
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 you create an object its identity changes for every render, forcing child components to re-render every time. useMemo
makes sure the object identity doesn't change unless one of the dependents (in the array) change. It was a specific warning from eslint for useContext
specifically, but this is generally good to do with React when passing down props.
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
==========================================
- Coverage 65.76% 65.73% -0.03%
==========================================
Files 50 50
Lines 7332 7332
==========================================
- Hits 4822 4820 -2
- Misses 1725 1726 +1
- Partials 785 786 +1 ☔ View full report in Codecov by Sentry. |
inputProps: { | ||
style: { paddingBottom: 0, paddingTop: 0 }, | ||
"aria-label": props.placeholder, | ||
}, | ||
}} |
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.
They should be adjacent, not nested:
InputProps={{
style: { fontSize: theme.typography[props.variant].fontSize },
}}
inputProps={{
style: { paddingBottom: 0, paddingTop: 0 },
"aria-label": props.placeholder,
}}
@@ -1008,8 +1003,6 @@ const TokenDeleteDialog = (props) => { | |||
console.log(`[Account] Error deleting token`, e); | |||
if (e instanceof UnauthorizedError) { | |||
session.resetAndRedirect(routes.login); | |||
} else { | |||
setError(e.message); | |||
} |
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.
This should be <DialogFooter status={error}>
, instead of deleting it
Thanks for the review and final fixes! The |
(Please look at the individual commit descriptions for more context)