@jakubfiglak
Submitted
My experiment with tRPC using t3 stack
@j0sephh123
@jakubfiglak
Submitted
My experiment with tRPC using t3 stack
@j0sephh123
Posted
Great job, looks very clean and maintainable. Works fine on the deployed url, haven't started it locally due to the need of postgres. I really like how you have used react-query, modules folder, hooks with optimistic updates and rollback.
// src/layout/AppLayout.tsx
type LayoutContentWrapperProps = {
className?: string;
} & PropsWithChildren;
…
export const AppLayout = ({ children }: PropsWithChildren) => {
router.query.status
in src/modules/todos/views/TodosView.tsx
, i have a small idea to create a custom hook. Since you are using router.query.status
in 2 files you may be able to abstract a bit.function useRouterQueryStatus<T>() {
const router = useRouter();
return router.query.status as T;
}
export const TodosView = ({ className }: TodosViewProps) => {
const status = useRouterQueryStatus<TodoStatus>();
useTodos
hook.export function useTodos(status?: TodoStatus) {
const { isLoading, data } = api.todos.getAll.useQuery({ status });
return { isLoading, todos: data };
}
@PauliMaenpaa
Submitted
I made all the buttons flash orange, if user tries to submit without rating.
@j0sephh123
Posted
Feedback: Great work so far! Your HTML structure is clean and well-organized, and your JavaScript code is easy to follow. Keep it up! I noticed a few areas where we can improve the consistency of your styling:
CSS units: It seems like you're using both pixels and rems in your code. To maintain consistency, I would suggest choosing one and sticking to it throughout your project. I personally prefer using rems, as they can provide a better responsive design.
Colors: Nice job defining CSS variables for colors! Just a quick note - there are still some instances of hard-coded colors in your code. To keep everything consistent and easy to manage, try to use the defined variables instead of hard coding the colors.
Duplicate classes: I noticed that you have two classes with the same styling: rating-container and submit-container. You could probably create a more generic class like flex-container or similar to consolidate these styles and reduce redundancy.
Repository: Looking at your GitHub repository, it would be a good idea to add a short README to explain what the code does and how to run it. This will make it easier for others to understand your project and contribute.
Marked as helpful
@Soaphub
Submitted
Did some changes to the initial design. Please let me know the responsive design.
@j0sephh123
Posted
There is duplicated code in pages/
. You can look into _app.js
how to have a single layout
To load a global css - you can use this article
Avoid accessing the DOM directly.
For example in Cart.js
you can set directly in the JSX
<p id="title-cart">{'set the stuff here'}</p>
, which you are currently doing inside useEffect
Marked as helpful
@faizov
Submitted
A little practice with redux toolkit rtk query. Change theme, use localStorage, prefers colors.
@j0sephh123
Posted
.map<React.ReactNode>
why typing that? It is inferred by TS compiler.src/copmonents/pageLayout/index.tsx
src/app/hooks.ts
, but you are still using the original onesstring
, here it'd be a good place to use enum or union type, since it can be only 2 valuesonChangeTheme
function is doing some logic inside the component
dispatch(setTheme(theme !== "light" ? "light" : "dark"));
I would suggest that you actually do that logic inside the reducer.src/pages/home/index.tsx
dataAll
and allLoading
are not good names for variables.byRegionLoading
is unused variableReact Hook useEffect has a missing dependency: 'closeOpenMenus'. Either include it or remove the dependency array.
Error
and Loading
.result.map
, you could do some destructuring there.const data: CountryApi | undefined
it would be good to handle the fetching in one component and only have a child component that receives data when we are certain it is only CountryApi. Thus, you can avoid checking on severa places if it is undefined.hideSelect &&
what will be here in a custom component, it would need to take only one prop handleChangeRegion
, since the listSelect is a hard coded array.region
to a union type, currently it is a string. You can have all the possible values here: src/pages/home/config.ts
src/copmonents/card/index.tsx
capital &&
you are checking for capital truthiness here, but even if it is an empty array, it is still truthy, so there is no need for that check. A better approach would be to check its length.src/@types/index.ts
src/pages/country/index.tsx
country
here. Create a child component, check if country is NOT undefined, then pass it to the child component, thus you will avoid all the ?
checks that you have for country.CountriesCode
CapitalCase?Marked as helpful
@Daniil034
Submitted
This is my first project where I work with external REST API resources. For this task I decided to use Redux'es createAsyncThunk because it gives you an opportunity to easily handle different states the application is in at the moment of hooking from external API. Working with React components again I found it quite disastrous to make styles for every component in a separate file and eventually I always got lost. So, I need to find any solution to alleviate my struggles during such projects. Any code review would be amazing. Thank you!
@j0sephh123
Posted
unwrap
for createAsyncThunk
, so thanks for introducing that to me :)src/routes/app/App.jsx
useEffect
seems to be missing dependencies. I suggest that you setup eslint and the hooks plugin - https://www.npmjs.com/package/eslint-plugin-react-hooks
It is pain to setup, but will warn you for missing or incorrect dependencies for useEffect, useMemo, useCallback and so on.src/components/Header/Header.jsx
- you can create enum for themes, instead of using raw stringssrc/components/InputElement/InputElement.jsx
- 'await' has no effect on the type of this expression
this is for setTimeout. Also, you can use the shorthand syntax for this: onChange={handleInput}
Also shorthand for this: {countryName && <div className="close" onClick={handleErase}></div>}
It is good that you have 1sec debounce after typing to avoid sending fast requests. You can also use https://usehooks.com/useDebounce/ or extract your own logic into a custom hook for reusability. (not that it is needed here)
src/features/regions/Regions.jsx
- hidden={countryRegion === '' ? true : false}
can be simply hidden={countryRegion === ''}
selected={countryRegion === region ? true : false}
firstRender
used for? It doesn't seem to do anything, you just set it in the useEffect.src/features/countries/Countries.jsx
In handleScroll
avoid lenghty statements inside if. Create a variable and use it inside if.
I will try to provide an idea. Note that there is a high probability that this syntax may not be working outside of the box, it is just to illustrate what I have in mind:// Countries
const Countries = () => {
...
const isLoading = useSelector(selectCountriesIsLoading);
useHandleScroll(isLoading, () =>
dispatch(
loadCountries({ countryName: null, countryRegion: null, scroll: true })
)
);
...
And a new hook file
//src/features/countries/useHandleScroll.js
export default function useHandleScroll(isLoading, callback) {
const handleScroll = () => {
const ourVariableName = window.innerHeight + Math.ceil(window.pageYOffset) >= document.body.offsetHeight && !isLoading;
if (ourVariableName) {
callback();
}
};
useEffect(() => {
window.addEventListener("scroll", handleScroll);
return () => window.removeEventListener("scroll", handleScroll);
}, [isLoading]);
}
Marked as helpful
@YerikAH
Submitted
Hello everyone, another challenge completed, this challenge was a bit difficult for me, if you think that the application has an error, I would really appreciate it if you could tell me about it.
@j0sephh123
Posted
Hello, adding some feedback
const [tipRender, setTipRender] = useState<tipPor[]>([]); // good
const [billState, setBillState] = useState<string>(""); // no need for <string>
const [erroBill, setErroBill] = useState<boolean>(false); // no need for <boolean>
const [styleErrBill, setStyleErrBill] = useState<styleErr>();
// here we could use null to handle when there is no error. It is a bit more verbose, but shows that error is null, i.e no error initially.
const [styleErrBill, setStyleErrBill] = useState<styleErr | null>(null);
trunc
in different files like helpers or similar, they don't need to be defined inside the component. Keep functions like handleClick
, which use components specific variables.useMessage("Can't be zero")
; - messages can be defined as variables and exported from another filevalidationInput
you have 3 levels deep ifs. Try to rewrite the function to avoid so much complexity.function calcSimpleTip(): void
if the function returns void, it doesn't need to be explicitly typed, it will be inferred from TS compiler.calcSimpleTip
why billStateConvert and the other variables are defined with let ? Firstly, they can be const, also their type will be inferred. For example
const billStateConvert = validationInput(billState, setErroBill, setErrMessBill); // will also have type number
let calctip = (billStateConvert * (porcentConvert / 100)) / peopleStateConvert;
Here calctip doesnt need to be casted to number, because its type is inferred from the expression. Typescript is smart.function resetStatusTip(): tipPor[]
doesn't need to have explicit return type.e
, but it is not used.Overall, good job. I would suggest the following:
Marked as helpful
@aleksFedotov
Submitted
Feedback will be highly appreciated
@j0sephh123
Posted
First, good job. There is a ton more stuff that can be discussed, but it would take a lot more time, the application is huge :)
I have started this challenge, but dunno if or when I'm going to finish it, because I underestimated it and thought would finish it more quickly. Seeing it finished, althought with extra stuff, only makes me realize how big it actually is.
I will mostly open points for discussion, since this is not code review, just commenting on various things. Will use points, so it would be easier if you decide to comment on these.
{windowWidth > 760 ? ..
For example for the following statement, you could extract 760 into a global variable, assign the statement into a variable also, since it is used twice in src/components/game/board/Board.tsx
Again, for the same component as far as I know React.FC is not necessary when the component doesn't receive props.
7. src/components/game/controlGrid/ControlGrid.tsx
- here you are comparing to variables ('p2','CPUvP'), these can easily be enums that can be re-used across the application.
8. Typescript - I suggest that you use CapitalCase for types. Also the types can be refined by using union types and enums.
@abhishekrathaur80
Submitted
Please provide feedback.
@j0sephh123
Posted
Hi, looks good, few remarks.
@media screen and(max-width:395) {
As a general suggestion, start styling for mobile device and then add styles for desktop. Try to use fewer breakpoints if possible.
Marked as helpful