As an (aspiring) Junior React developer it’s hard to understand if you’re code is any good and how you can improve it. It would be great to get a review from an experienced dev. But if you don’t work in a team or have the funds to pay a mentor your chances are low.
Still, you can learn from reviews of other developers’ code. And that’s the goal of this series of articles and videos.
This time, we step by step review and refactor the code of one of my students. By
- using a single source of truth
- replacing a buggy feature with a simpler solution
- and removing programmatic styles in favor of simple CSS
we greatly reduce the complexity of the code, simplify the data flow, and flush out a few bugs along the way.
If you’re rather a video person you can watch the complete review and refactoring session here.
If you’re interested in getting a code review like this yourself feel free to reach out to me at review@profy.dev. I’m always looking for more examples. To increase your chances request a review of one feature rather than a whole application.
Table Of Contents
- The Component To Review
- Code Review: The Filter Values
- Refactoring: A Single Source Of Truth
- Code Review: Incorrect Default Values
- Code Review: The Project Name Autocomplete
- Code Review: Programmatic Styles
- Final Result
The Component To Review
The UI & Functionality
The feature to be reviewed is the filter bar as highlighted in this screenshot. It is used to filter the data in the table.
This happens when the user selects a filter (try it out yourself here):
- a new request is sent to an API
- the table updates with the new data
- the selected filter value is added to the URL (as shown below).
The first impression: The UI looks very good. The functionality also works as expected. At least at first glance.
The Filters Component
Now that we’ve seen the UI let’s have a look at the code. Here is the Filters component:
Try to review the code yourself first. What problems do you see and can you find alternative solutions? You can find the complete code here on GitHub.
import React, {
useState,
useEffect,
useCallback,
useRef,
useContext,
} from "react";
import { useRouter } from "next/router";
import { useWindowSize } from "react-use";
import { Select, Option, Input, NavigationContext } from "@features/ui";
import { useFilters } from "../../hooks/use-filters";
import { IssueLevel, IssueStatus } from "@api/issues.types";
import { useProjects } from "@features/projects";
import * as S from "./filters.styled";
export function Filters() {
const { handleFilters, filters } = useFilters();
const { data: projects } = useProjects();
const router = useRouter();
const routerQueryProjectName =
(router.query.projectName as string)?.toLowerCase() || undefined;
const [inputValue, setInputValue] = useState<string>("");
const projectNames = projects?.map((project) => project.name.toLowerCase());
const isFirst = useRef(true);
const { width } = useWindowSize();
const isMobileScreen = width <= 1023;
const { isMobileMenuOpen } = useContext(NavigationContext);
const handleChange = (input: string) => {
setInputValue(input);
if (inputValue?.length < 2) {
handleProjectName(undefined);
return;
}
const name = projectNames?.find((name) =>
name?.toLowerCase().includes(inputValue.toLowerCase())
);
if (name) {
handleProjectName(name);
}
};
const handleLevel = (level?: string) => {
if (level) {
level = level.toLowerCase();
}
handleFilters({ level: level as IssueLevel });
};
const handleStatus = (status?: string) => {
if (status === "Unresolved") {
status = "open";
}
if (status) {
status = status.toLowerCase();
}
handleFilters({ status: status as IssueStatus });
};
const handleProjectName = useCallback(
(projectName?: string) =>
handleFilters({ project: projectName?.toLowerCase() }),
[handleFilters]
);
useEffect(() => {
const newObj: { [key: string]: string } = {
...filters,
};
Object.keys(newObj).forEach((key) => {
if (newObj[key] === undefined) {
delete newObj[key];
}
});
const url = {
pathname: router.pathname,
query: {
page: router.query.page || 1,
...newObj,
},
};
if (routerQueryProjectName && isFirst) {
handleProjectName(routerQueryProjectName);
setInputValue(routerQueryProjectName || "");
isFirst.current = false;
}
router.push(url, undefined, { shallow: false });
}, [filters.level, filters.status, filters.project, router.query.page]);
return (
<S.Container>
<Select
placeholder="Status"
defaultValue="Status"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</Select>
<Select
placeholder="Level"
defaultValue="Level"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleLevel}>
--None--
</Option>
<Option value="Error" handleCallback={handleLevel}>
Error
</Option>
<Option value="Warning" handleCallback={handleLevel}>
Warning
</Option>
<Option value="Info" handleCallback={handleLevel}>
Info
</Option>
</Select>
<Input
handleChange={handleChange}
value={inputValue}
label="project name"
placeholder="Project Name"
iconSrc="/icons/search-icon.svg"
style={{
...(isMobileScreen && { width: "94%", marginRight: "3rem" }),
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
/>
</S.Container>
);
}
There’s a lot going on. Especially the top of the component is pretty crowded with lots of variables with unclear responsibilities. So let’s review and refactor the code one by one.
Code Review: The Filter Values
We start with the elephant in the room: The filter values. Let’s try to answer some basic questions regarding the data flow: How are the filter values used, how are they updated, and where are they stored?
How Are The Filter Values Used?
The filters object comes from the useFilters() hook and is only used in the useEffect to update the query params in the URL.
Note: The code is a bit easier to read in the original article since the important lines are highlighted there. This is not supported here as far as I know.
// ./components/filters.tsx
export function Filters() {
const { handleFilters, filters } = useFilters();
useEffect(() => {
const newObj: { [key: string]: string } = {
...filters,
};
Object.keys(newObj).forEach((key) => {
if (newObj[key] === undefined) {
delete newObj[key];
}
});
const url = {
pathname: router.pathname,
query: {
page: router.query.page || 1,
...newObj,
},
};
if (routerQueryProjectName && isFirst) {
handleProjectName(routerQueryProjectName);
setInputValue(routerQueryProjectName || "");
isFirst.current = false;
}
router.push(url, undefined, { shallow: false });
}, [filters.level, filters.status, filters.project, router.query.page]);
...
// interestingly the filters aren't used in the components at all
return (
<S.Container>
<Select
placeholder="Status"
defaultValue="Status"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</Select>
...
</S.Container>
);
}
Interestingly the filters aren’t used in any of the component props. And as we’ll see later, this causes a bug.
How Are The Filter Values Updated?
The handleFilters function that is returned by the useFilters hook is used in the change callbacks. These are connected to the Select and Input components.
// ./components/filters.tsx
export function Filters() {
const { handleFilters, filters } = useFilters();
...
const handleLevel = (level?: string) => {
if (level) {
level = level.toLowerCase();
}
handleFilters({ level: level as IssueLevel });
};
const handleStatus = (status?: string) => {
if (status === "Unresolved") {
status = "open";
}
if (status) {
status = status.toLowerCase();
}
handleFilters({ status: status as IssueStatus });
};
const handleProjectName = useCallback(
(projectName?: string) =>
handleFilters({ project: projectName?.toLowerCase() }),
[handleFilters]
);
...
// interestingly the filters aren't used in the components at all
return (
<S.Container>
<Select
placeholder="Status"
defaultValue="Status"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</Select>
...
</S.Container>
);
}
Note that the change callbacks look a bit strange as they have to transform the input parameter before using it as a filter value. The Select component also looks counterintuitive with the
handleCallbackprop being set on theOptioncomponent. But that’s a topic for later.
Indirectly handleFilters is also called inside the useEffect whenever there’s a projectName query param in the URL.
// ./components/filters.tsx
export function Filters() {
const router = useRouter();
const routerQueryProjectName =
(router.query.projectName as string)?.toLowerCase() || undefined;
const isFirst = useRef(true);
...
useEffect(() => {
const newObj: { [key: string]: string } = {
...filters,
};
Object.keys(newObj).forEach((key) => {
if (newObj[key] === undefined) {
delete newObj[key];
}
});
const url = {
pathname: router.pathname,
query: {
page: router.query.page || 1,
...newObj,
},
};
if (routerQueryProjectName && isFirst) {
handleProjectName(routerQueryProjectName);
setInputValue(routerQueryProjectName || "");
isFirst.current = false;
}
router.push(url, undefined, { shallow: false });
}, [filters.level, filters.status, filters.project, router.query.page]);
return (
<S.Container>
...
</S.Container>
);
}
After some testing the highlighted line never seems to be executed though. As mentioned, the rest of the useEffect is responsible for updating the query parameters in the URL whenever a filter changes.
There’s also a small bug as
isFirstin the condition is always truthy.isFirst.currentshould have been used instead.
Where Are The Filter Values Stored?
Now we know how the filters object and the handleFilters function are used. But where are they defined? This is a good time to have a look at the useFilters hook. The hook itself simply exposes a context.
// ./hooks/use-filters.ts
import { useContext } from "react";
import { FiltersContext } from "../context/filters-context";
export const useFilters = () => useContext(FiltersContext);
The context itself isn’t very complex either. Basically, it stores the filter values in a state and exposes a function to update this state (aka the handleFilters function).
// ./context/filters-context.tsx
import React, {
useState,
useMemo,
useCallback,
createContext,
ReactNode,
} from "react";
import { IssueFilters } from "@api/issues.types";
export const FiltersContext = createContext<{
filters: IssueFilters;
handleFilters: (filter: IssueFilters) => unknown;
}>({
filters: { status: undefined, level: undefined, project: undefined },
handleFilters: (_filter: IssueFilters) => {},
});
type FiltersProviderProps = {
children: ReactNode | ReactNode[];
};
export function FiltersProvider({ children }: FiltersProviderProps) {
const [filters, setFilters] = useState<IssueFilters>({
status: undefined,
level: undefined,
project: undefined,
});
const handleFilters = useCallback(
(filter: any) =>
setFilters((prevFilters) => ({ ...prevFilters, ...filter })),
[]
);
const memoizedValue = useMemo(
() => ({ filters, handleFilters }),
[filters, handleFilters]
);
return (
<FiltersContext.Provider value={memoizedValue}>
{children}
</FiltersContext.Provider>
);
}
There are likely some opportunities for refactoring here. For example, the usage of useCallback and useMemo might not be necessary, and some of the types could be improved (esp. any). But as we’ll see in a bit we can completely replace this code with a simpler solution.
To get to that solution, let’s have a closer look at the data flow.
The Data Flow
The current filters data is defined in a context and in the query parameters in the URL. Here you can see how this data changes:
- After the initial render the filters are undefined both inside the context and the URL.
- Whenever the user changes a filter value the state in the context is updated.
- The state update triggers a re-render and executes the
useEffect. This in turn updates the query parameters in the URL viarouter.push(...).
The last re-render after the update of the URL isn’t really important at the moment. But it could become a performance issue in the future. So we should keep it in mind.
The Core Problem: Duplicate Data
The core problem that leads to a lot of complexity including the useEffect is the duplication of the filter values:
- The filters are stored in a state inside a context.
- The filters are also stored in the URL.
The question is: Can we either get rid of the state or the URL parameters?
Removing the URL parameters isn’t an option. They are a hard requirement for the feature as they make the filter values persist a page refresh and allow the user to share a certain filtered view with another user.
But what about the state?
An Alternative Solution
Can we simply derive the filter values from the URL? E.g. like this:
const router = useRouter();
const filters = {
status: router.query.status,
...
}
That seems worth a try. When a user changes a filter value we could directly update the URL. And that in turn would update the derived filters object. The data flow would look like this:
This might not seem like a big change. But as you’ll see during the refactoring this will greatly simplify our code as we don’t need the context and the synchronization logic in the useEffect anymore.
Refactoring: A Single Source Of Truth
Deriving The Filters Values From The URL
Based on the proposal for the alternative solution we directly derive the filters variable from the URL’s query parameters instead of using the context. We can do all this in the useFilters hook.
// ./hooks/use-filters.ts
import { useRouter } from "next/router";
import { IssueFilters } from "@api/issues.types";
export const useFilters = () => {
const router = useRouter();
const filters = {
status: router.query.status,
level: router.query.level,
project: router.query.project,
} as IssueFilters;
return { filters };
};
Updating The Filters Directly In The URL
Next, we add the handleFilters function. It accepts a new filters object, builds a new query from this object and the existing query, and updates the URL via router.push(...).
// ./hooks/use-filters.ts
import { useRouter } from "next/router";
import { IssueFilters } from "@api/issues.types";
export const useFilters = () => {
const router = useRouter();
const filters = {
status: router.query.status,
level: router.query.level,
project: router.query.project,
} as IssueFilters;
const handleFilters = (newFilters: IssueFilters) => {
const query = { ...router.query, ...newFilters };
router.push({ query });
};
return { filters, handleFilters };
};
Now that the hook doesn’t use the context anymore we can completely remove it (including the old filters state). Of course, we also need to remove all references to the context from the rest of the app (e.g. the FiltersProvider). But I won’t show that here.
A Layer Of Abstraction Made The Refactoring Simple
There’s one thing that’s easy to miss: the author of the original code did a great job of introducing a layer of abstraction by creating the useFilters hook.
We were able to refactor the internals of the hook and get rid of the context and its state. But since we return the same object from the hook with the same filters object and handleFilters function as before we didn’t change the API of the hook.
This way we were able to change the complete filters implementation without changing a single line in the Filters component.
This is a great example of how custom hooks can greatly improve the maintainability of your code. Kudos to the author.
Removing The useEffect
While we don’t need to change the Filters component we can get rid of one of the main complexities in the component: the useEffect that was responsible for updating the query parameters in the URL.
// ./components/filters.tsx
export function Filters() {
const { handleFilters, filters } = useFilters();
const [inputValue, setInputValue] = useState<string>("");
const projectNames = projects?.map((project) => project.name.toLowerCase());
const { data: projects } = useProjects();
const { width } = useWindowSize();
const isMobileScreen = width <= 1023;
const { isMobileMenuOpen } = useContext(NavigationContext);
const handleChange = (input: string) => {...};
const handleLevel = (level?: string) => {...};
const handleStatus = (status?: string) => {...};
const handleProjectName = useCallback(...);
return (
<S.Container>
...
</S.Container>
);
}
Recap
By simply deriving the filter values directly from the URL instead of keeping them in a state we were able to reduce the complexity and remove ~80 lines of code (the useEffect and the context). You can find the complete code changes here on GitHub.
This was possible because of our analysis of the data flow and a simple change to storing the filter values. The rest of this review & refactoring session won’t be that impactful to be honest. So you’re key takeaway from this article should be:
Use a single source of truth whenever possible. You’ll get around a lot of complicated and error-prone syncing logic (the useEffect in our case). Duplicate data is very common and might not be easy to detect. But whenever you encounter a useEffect pay close attention.
Now lets go ahead with the next step of the code review.
Code Review: Incorrect Default Values
As we’ve already seen, the filter values aren’t used in any of the Select or Input components. The result is that these components aren’t initialized with the correct values from the URL. They always show a hard-coded default value.
The Problem
This seems like an easy fix at first. And for the “Project Name” input that’s true as it is controlled via a state in the Filters component.
export function Filters() {
const { handleFilters, filters } = useFilters();
const [inputValue, setInputValue] = useState("");
...
But for the Select components, this turns out to be a bigger problem. Currently, the defaultValue prop is hard-coded.
<Select
placeholder="Status"
defaultValue="Status"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</Select>
Ideally, we could control the Select component via a value prop. But unfortunately, its current implementation doesn’t allow that. So we’ll have to find a workaround until we change the Select component itself in an upcoming refactoring session.
Refactoring: Initialize The Input Elements
As mentioned, initializing the “Project Name” input is simple as this is controlled by the inputValue state.
// ./components/filters.tsx
export function Filters() {
const { handleFilters, filters } = useFilters();
const [inputValue, setInputValue] = useState(filters.project || "");
...
But fixing the initial value of the Select components are a bit more difficult as we can only use the defaultValue prop.
We could obviously refactor the Select component itself but that’s outside the scope of this session. We will tackle that problem in an upcoming refactoring session though.
To get the default value for each Select we introduce two functions. Those return the current default value (”Status” and “Level”) if the corresponding filter is not set. Otherwise, they transform the filter value to match the text rendered in the corresponding Option component.
// ./components/filters.tsx
import { capitalize } from "lodash";
import { IssueFilters, IssueLevel, IssueStatus } from "@api/issues.types";
...
function getStatusDefaultValue(filters: IssueFilters) {
if (!filters.status) {
return "Status";
}
if (filters.status === IssueStatus.open) {
return "Unresolved";
}
return "Resolved";
}
function getLevelDefaultValue(filters: IssueFilters) {
if (!filters.level) {
return "Level";
}
return capitalize(filters.level);
}
export function Filters() {
const { handleFilters, filters } = useFilters();
...
return (
<S.Container>
<Select
placeholder="Status"
defaultValue={getStatusDefaultValue(filters)}
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</Select>
<Select
placeholder="Level"
defaultValue={getLevelDefaultValue(filters)}
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
<Option value={undefined} handleCallback={handleLevel}>
--None--
</Option>
<Option value="Error" handleCallback={handleLevel}>
Error
</Option>
<Option value="Warning" handleCallback={handleLevel}>
Warning
</Option>
<Option value="Info" handleCallback={handleLevel}>
Info
</Option>
</Select>
...
</S.Container>
);
}
Having to use the functions getStatusDefaultValue and getLevelDefaultValue isn’t really pretty here. But during a refactoring, it’s important to advance step by step. We can still remove these functions once we have a better implementation of the Select component.
You can find all changes of this bugfix here on GitHub. Now let’s continue with the next part of the code review.
Code Review: The Project Name Autocomplete
The change handler connected to the project name input has some kind of autocomplete functionality. Below you can see it in action: The user only enters a part of the project name in the input but the complete name (”Frontend - Web”) is added to the URL.
What happens here? The Filters component loaded the existing projects from the API. When the user starts typing, the code tries to find a project that matches the given input. If there is one it uses its name as a query parameter.
Here is the corresponding code:
// ./components/filters.tsx
export function Filters() {
...
const { data: projects } = useProjects();
const projectNames = projects?.map((project) => project.name.toLowerCase());
const [inputValue, setInputValue] = useState<string>("");
const handleChange = (input: string) => {
setInputValue(input);
if (inputValue?.length < 2) {
handleProjectName(undefined);
return;
}
const name = projectNames?.find((name) =>
name?.toLowerCase().includes(inputValue.toLowerCase())
);
if (name) {
handleProjectName(name);
}
};
const handleProjectName = useCallback(
(projectName?: string) =>
handleFilters({ project: projectName?.toLowerCase() }),
[handleFilters]
);
...
The Problem
The idea behind this code is very good and could lead to an improved user experience. But the implementation has some problems:
- What if there are two projects that match the input? The code would take the first and the table would only show the data belonging to this one project. The second project’s data would be omitted.
- The user doesn’t see that a particular project was selected. This would only add to the confusion.
A better implementation would show the different matches to the user similar to the Google search interface.
But building such a feature would be more complex of course.
Refactoring: Remove Project Input Autocomplete
As mentioned, the autocomplete is a good idea in general. But it’s better to remove this functionality altogether rather than leave the user confused. If this was an important feature I’d go back to the whiteboard and sketch out a proper solution that includes user feedback for better UX.
The simplest solution is updating the filters with every keystroke.
// ./components/filters.tsx
export function Filters() {
const { handleFilters, filters } = useFilters();
const handleChange = (project: string) => {
handleFilters({ project });
};
...
This works but causes a re-render of the whole page whenever the user changes the input value. In our case, the performance implications of these re-renders aren’t that important though. The bigger problem is that every keystroke also triggers a new API request.
To reduce the number of requests we can debounce the updating of the filters. There’s a great small npm package called use-debounce that we can use to create a debounced version of the handleFilters function.
// ./components/filters.tsx
import { useDebouncedCallback } from "use-debounce";
...
export function Filters() {
const { handleFilters, filters } = useFilters();
const debouncedHandleFilters = useDebouncedCallback(handleFilters, 300);
const [inputValue, setInputValue] = useState(filters.project || "");
const handleChange = (project: string) => {
setInputValue(project);
debouncedHandleFilters({ project });
};
...
Now you can see that the project parameter in the URL only updates shortly after the user stopped typing.
You can find all code changes here on GitHub.
Code Review: Programmatic Styles
The last point of this code review is the styles. The application uses styled-components as a CSS solution. So we should be able to style almost anything with styled-components.
But interestingly the Filters component also uses conditional styles within the JSX:
// ./components/filters.tsx
export function Filters() {
const { width } = useWindowSize();
const isMobileScreen = width <= 1023;
const { isMobileMenuOpen } = useContext(NavigationContext);
...
return (
<S.Container>
<Select
placeholder="Status"
defaultValue="Status"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
...
</Select>
<Select
placeholder="Level"
defaultValue="Level"
width={isMobileScreen ? "97%" : "8rem"}
style={{
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
>
...
</Select>
<Input
handleChange={handleChange}
value={inputValue}
label="project name"
placeholder="Project Name"
iconSrc="/icons/search-icon.svg"
style={{
...(isMobileScreen && { width: "94%", marginRight: "3rem" }),
...(isMobileMenuOpen && {
opacity: 0,
}),
}}
/>
</S.Container>
);
}
The Problem
- The
SelectandInputcomponents are styled depending on the window width. The magic number1023seems to be the desktop breakpoint. Using magic numbers in general is not recommended. But why don’t we use CSS media queries in the first place? - The
SelectandInputcomponents are basically set invisible if the navigation menu is open on mobile devices. This seems like a hacky solution to a problem that the author of this code encountered. So let’s see if we can find a proper alternative.
Refactoring: Fix Select Overlay Workaround
Next, let’s get rid of the programmatic styles. The first problem that we have to fix are the conditional styles when the mobile navigation menu is open. Let’s see what happens when we remove the following lines of code:
// ./components/filters.tsx
export function Filters() {
...
return (
<S.Container>
<Select
placeholder="Status"
defaultValue={getStatusDefaultValue(filters)}
width={isMobileScreen ? "97%" : "8rem"}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</Select>
<Select
placeholder="Level"
defaultValue={getLevelDefaultValue(filters)}
width={isMobileScreen ? "97%" : "8rem"}
>
<Option value={undefined} handleCallback={handleLevel}>
--None--
</Option>
<Option value="Error" handleCallback={handleLevel}>
Error
</Option>
<Option value="Warning" handleCallback={handleLevel}>
Warning
</Option>
<Option value="Info" handleCallback={handleLevel}>
Info
</Option>
</Select>
<Input
handleChange={handleChange}
value={inputValue}
label="project name"
placeholder="Project Name"
iconSrc="/icons/search-icon.svg"
style={{
...(isMobileScreen && { width: "94%", marginRight: "3rem" }),
}}
/>
</S.Container>
);
}
When we open the app in the responsive view we can see that the select and input elements overlay the mobile navigation menu.
Investigating the navigation menu via the Chrome dev tools shows that the only required change is setting a z-index to one of its parents.
Debugging CSS can be really difficult and annoying. But as you can see here finding the root cause of a styling issue helps avoid cumbersome workarounds.
You can find all changes here on GitHub.
Refactoring: Replace Programmatic Mobile Styles With CSS
As a final step in this refactoring let’s replace the remaining programmatic styles with simple CSS and media queries.
// ./components/filters.tsx
import * as S from "./filters.styled";
...
export function Filters() {
const { handleFilters, filters } = useFilters();
const debouncedHandleFilters = useDebouncedCallback(handleFilters, 300);
const [inputValue, setInputValue] = useState(filters.project || "");
- const { width } = useWindowSize();
- const isMobileScreen = width <= 1023;
...
return (
<S.Container>
<S.Select
placeholder="Status"
defaultValue={getStatusDefaultValue(filters)}
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</S.Select>
<S.Select
placeholder="Level"
defaultValue={getLevelDefaultValue(filters)}
>
<Option value={undefined} handleCallback={handleLevel}>
--None--
</Option>
<Option value="Error" handleCallback={handleLevel}>
Error
</Option>
<Option value="Warning" handleCallback={handleLevel}>
Warning
</Option>
<Option value="Info" handleCallback={handleLevel}>
Info
</Option>
</S.Select>
<S.Input
handleChange={handleChange}
value={inputValue}
label="project name"
placeholder="Project Name"
iconSrc="/icons/search-icon.svg"
/>
</S.Container>
);
}
This project uses styled-components so the new S.Select and S.Input components look like this.
// ./components/filters.styled.tsx
import styled from "styled-components";
import { breakpoint } from "@styles/theme";
import { Select as UnstyledSelect, Input as UnstyledInput } from "@features/ui";
...
export const Select = styled(UnstyledSelect)`
width: 97%;
@media (min-width: ${breakpoint("desktop")}) {
width: 8rem;
}
`;
export const Input = styled(UnstyledInput)`
width: 94%;
margin-right: 3rem;
@media (min-width: ${breakpoint("desktop")}) {
width: 17.5rem;
margin-right: 0;
}
`;
This is where we hit a roadblock. The internal implementation of the Select component prevents the width defined in the CSS from being applied. Here you can see that its width is hard-coded internally:
You can find all changes here on GitHub.
Final Result
Below you can find the final code after this refactoring session. You can also see it here on GitHub.
We were able to remove a lot of complexity by using the URL as single source of truth for the filter values. This allowed us to get rid of a context. The resulting custom hook useFilters is very straightforward:
// ./hooks/use-filters.ts
import { useRouter } from "next/router";
import { IssueFilters } from "@api/issues.types";
export const useFilters = () => {
const router = useRouter();
const filters = {
status: router.query.status,
level: router.query.level,
project: router.query.project,
} as IssueFilters;
const handleFilters = (newFilters: IssueFilters) => {
const query = { ...router.query, ...newFilters };
router.push({ query });
};
return { filters, handleFilters };
};
The Filters component was also simplified especially by removing the useEffect.
// ./components/filters.tsx
import React, { useState } from "react";
import { useDebouncedCallback } from "use-debounce";
import { capitalize } from "lodash";
import { Option } from "@features/ui";
import { IssueFilters, IssueLevel, IssueStatus } from "@api/issues.types";
import { useFilters } from "../../hooks/use-filters";
import * as S from "./filters.styled";
function getStatusDefaultValue(filters: IssueFilters) {
if (!filters.status) {
return "Status";
}
if (filters.status === IssueStatus.open) {
return "Unresolved";
}
return "Resolved";
}
function getLevelDefaultValue(filters: IssueFilters) {
if (!filters.level) {
return "Level";
}
return capitalize(filters.level);
}
export function Filters() {
const { handleFilters, filters } = useFilters();
const debouncedHandleFilters = useDebouncedCallback(handleFilters, 300);
const [inputValue, setInputValue] = useState(filters.project || "");
const handleChange = (project: string) => {
setInputValue(project);
debouncedHandleFilters({ project });
};
const handleLevel = (level?: string) => {
if (level) {
level = level.toLowerCase();
}
handleFilters({ level: level as IssueLevel });
};
const handleStatus = (status?: string) => {
if (status === "Unresolved") {
status = "open";
}
if (status) {
status = status.toLowerCase();
}
handleFilters({ status: status as IssueStatus });
};
return (
<S.Container>
<S.Select
placeholder="Status"
defaultValue={getStatusDefaultValue(filters)}
data-cy="filter-by-status"
>
<Option value={undefined} handleCallback={handleStatus}>
--None--
</Option>
<Option value="Unresolved" handleCallback={handleStatus}>
Unresolved
</Option>
<Option value="Resolved" handleCallback={handleStatus}>
Resolved
</Option>
</S.Select>
<S.Select
placeholder="Level"
defaultValue={getLevelDefaultValue(filters)}
data-cy="filter-by-level"
>
<Option value={undefined} handleCallback={handleLevel}>
--None--
</Option>
<Option value="Error" handleCallback={handleLevel}>
Error
</Option>
<Option value="Warning" handleCallback={handleLevel}>
Warning
</Option>
<Option value="Info" handleCallback={handleLevel}>
Info
</Option>
</S.Select>
<S.Input
handleChange={handleChange}
value={inputValue}
label="project name"
placeholder="Project Name"
iconSrc="/icons/search-icon.svg"
data-cy="filter-by-project"
/>
</S.Container>
);
}
Unfortunately, we still need the functions getStatusDefaultValue and getLevelDefaultValue since the Select component can’t be controlled and doesn't handle the option value and display text separately yet. So a refactoring of the Select component would allow us to further simplify this code. But since this session already turned out quite long we’ll leave that for another time.
Top comments (1)
Dose Code Refactoring really helps people to give a new structure to a program