Skip to content
OverviewSolutions
0
Comments
8
j0sephh123
@j0sephh123

All comments

  • j0sephh123•140
    @j0sephh123Posted over 1 year ago

    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.

    • Small idea for Typescript:
    // src/layout/AppLayout.tsx
type LayoutContentWrapperProps = {

className?: string;

} & PropsWithChildren;
…
export const AppLayout = ({ children }: PropsWithChildren) => {
    • Seeing that you cast 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>();
    • Slightly opinionated idea for useTodos hook.
    export function useTodos(status?: TodoStatus) {
  const { isLoading, data } = api.todos.getAll.useQuery({ status });

  return { isLoading, todos: data };
}
    0
  • j0sephh123•140
    @j0sephh123Posted over 1 year ago

    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
    1
  • Ambadi M P•380
    @SoaphubSubmitted almost 2 years ago

    Did some changes to the initial design. Please let me know the responsive design.

    E-comerce

    2
    j0sephh123•140
    @j0sephh123Posted almost 2 years ago

    • 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
    1
  • j0sephh123•140
    @j0sephh123Posted about 2 years ago
    • Why create-react-app ? It is perfectly fine, just curious.
    • .map<React.ReactNode> why typing that? It is inferred by TS compiler.
    • Noticed that in some components you are importing React, I don't think it is necessary

    src/copmonents/pageLayout/index.tsx

    • you are defining the typed hooks from redux docs in here src/app/hooks.ts, but you are still using the original ones
    • for theme you have type string, here it'd be a good place to use enum or union type, since it can be only 2 values
    • onChangeTheme 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 variable
    • you could great benefit from es-lint, it gives me a few warning, such as not returning a value from filter function, React Hook useEffect has a missing dependency: 'closeOpenMenus'. Either include it or remove the dependency array.
    • you would benefit from creating components for Error and Loading.
    • there is a lot of repetition in result.map, you could do some destructuring there.
    • Since 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.
    • probably can extract 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.
    • I think that you can type 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

    • you could add the type for capital also

    src/pages/country/index.tsx

    • See here you are using again Loading and Error. You are repeating yourself - extract them as components
    • Same note for 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.
    • why is CountriesCode CapitalCase?
    Marked as helpful
    2
  • Daniil•170
    @Daniil034Submitted about 2 years ago

    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!

    REST countries API [react, react router v6, redux, asyncThunks]

    1
    j0sephh123•140
    @j0sephh123Posted about 2 years ago
    • When you need to use logic in react components, but don't need to return JSX, as is in src/components/ScrollToTop/ScrollToTop.jsx - create a custom hook
    • Haven't used 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.
    • Regarding your struggle with styles - you can look into SCSS modules, Tailwind or https://stitches.dev/. Really depends on your style. Personally I'm using SCSS modules at work and working on a project here with stitches and it is very nice - very convenient for a global theme. A bit hard to get into.
    • src/components/Header/Header.jsx - you can create enum for themes, instead of using raw strings
    • src/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 === ''}
      • same for selected={countryRegion === region ? true : false}
      • what is 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
    1
  • j0sephh123•140
    @j0sephh123Posted about 2 years ago

    Hello, adding some feedback

    • regarding typing in useState
    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);
    • Don't add types inside React components, either on top of the file or in another file depending on if it is used in multiple places or not.
    • const initialState - defined that also outside of the component
    • use CapitalCase for typescript type names
    • extract functions like 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 file
    • avoid deeply nested if's - in validationInput 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.
    • in 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
    • Avoid casting variables to a type. It should be used only in rare cases, when there are no other options ( casting is ) 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.
    • Again function resetStatusTip(): tipPor[] doesn't need to have explicit return type.
    • Regarding PartOneInput. Doesn't need to have type='text' since it it the default input prop. onBlur and onClick have e , but it is not used.

    Overall, good job. I would suggest the following:

    • split the application in smaller components
    • use useReducer function to handle the logic
    • refine Typescript knowledge a bit - it is endless, really, just a bit.
    Marked as helpful
    1
  • j0sephh123•140
    @j0sephh123Posted about 2 years ago

    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.

    1. What is the reason for using create-react-app, since it is a bit legacy. Is it because it has testing pre-configured and you don't have to do it manually or there is another reason.
    2. I see that you are using react-router. Are there any particular benefits that I'm missing here, because it seems that it is bringing additional complexity to the application. Could you explain what.
    3. I see that styled components have theme, which you can use for global variables instead of using css var(). Any reason for that?
    4. Haven't had the need to use animation library for react. What is the reasoning for using it? Have you used other libraries and decided to choose this or just picked that and stuck with it ?
    5. Could you explain why are you using a web worked in this case.
    6. There are parts that would be good to be refined for example using
    {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.

    2
  • j0sephh123•140
    @j0sephh123Posted about 2 years ago

    Hi, looks good, few remarks.

    • You last rule doesn't seem to be working, because it is missing pixels after 395 @media screen and(max-width:395) {
    • Below 820px, the heading is way too close to the picture.
    • There is no space between 10k+ companies 314 templates and 12M+ queries on mobile.
    • Avoid styling the body
    • You have main and master branches. I suggest you pick only one of the two and delete the other one. This is because when someone opens the repository, sees only README.md and 2 .jpg files.
    • You should install Prettier - this is an extension for VSCode. It is used to format the code. You can format both .html and .css files by using "Ctrl + Shift + P" -> Format document with -> Prettier. Also you can activate the option to format on save in VSCode, which is located in Settings.
    • I see that you have various colors, you can look into using CSS variables instead of using the colors directly in your css rules.
    • The heading font-family seems to be Times New Roman instead of the expected Inter. I see that you have that rule, it is because you were missing comma here: "font-family: "Inter" sans-serif;"
    • Font-family seems correct for the paragraph

    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
    0