Frontend Challenge Implementation by AllanMicuanski · Pull Request #52 · sizebay/frontend-challenge

AllanMicuanski · 2026-03-06T18:50:07Z

Implementation of the proposed frontend challenge.

Looking forward to your feedback.

progressBar

lesgo

Fix/task bar

fix: responsivenes

feat: add pnpm | mise

docs: added readme

AllanMicuanski · 2026-03-07T18:22:37Z

Thank you for the feedback and for the time dedicated to the review. I carefully analyzed all the comments and implemented the adjustments accordingly.

✅ Summary of Changes
Removed the unused Button component (YAGNI principle)
Fixed UX interaction issues: cursor feedback, filter toggle behavior, border alignment, and edit consistency
Refactored ConfettiAnimation Storybook configuration: fullscreen layout, play function, and autodocs improvements
Introduced a design token strategy using clsx and centralized semantic constants for colors, spacing, typography, and animation — eliminating magic values across components
Split TaskItem into smaller components: TaskActions and TaskTooltip
Refactored tests to focus on user-visible behavior rather than implementation details

🧪 Testing Strategy
Unit tests were refactored with Jest + React Testing Library, removing CSS class assertions and low-value mocks in favor of observable behavior and accessibility. 7 E2E tests were added with Playwright, covering the main user flows: create, complete, filter, search, edit, delete, input validation, and persistence.

- Install clsx library (v2.1.1) for proper className handling - Refactor TaskItem: use clsx for container and span dynamic classes - Refactor TaskForm: use clsx for button and svg dynamic opacity - Refactor Input: use clsx for conditional error styling - Eliminates extra spaces from template literal concatenation - Improves code readability and maintainability Addresses tech lead's comment: "use clsx or cn (shadcn pattern)" to avoid fragile className concatenation with empty string defaults

@theme

- Add --color-gray-50 token to design system - Replace hardcoded hex colors with semantic Tailwind tokens across components: * TaskItem: border-primary, text-text-primary, bg-danger, bg-success, bg-gray-medium, bg-gray-50 * TaskForm: bg-gray-lighter, border-gray-light, text-text-primary, bg-primary * FilterBar: bg-white, border-gray-light, focus:border-primary, text-primary, bg-gray-lighter * ProgressBar: bg-border, bg-success * ErrorBoundary: bg-background, text-danger, bg-primary, bg-gray-lighter * SearchBar: border-gray-light, focus:border-primary * TaskListSkeleton: bg-gray-50, border-gray-light, bg-border * Input: clsx refactoring for conditional styles - Standardize SVG icons in TaskItem: * Use consistent filled circle structure in both editing and selected modes * Match stroke colors to button backgrounds (#E34F4F for danger, #5DE290 for success) - Improve maintainability: centralized color management in index.css @theme block

- Change layout from 'centered' to 'fullscreen' for proper viewport coverage - Add play function to Visible story documenting auto-play behavior - Move detailed description to component JSDoc for autodocs integration - Enhance argTypes with control descriptions

- Added clsx to technologies section - Highlighted design tokens implementation - Updated TaskForm icon color to match primary brand color (#4DA6B3)

App.tsx: remove useState callback anti-pattern TaskList: use useRef instead of callback for deselection SearchBar: delete unused component (-73 lines) Header: fix month to uppercase Addresses PR sizebay#52 feedback point #1

- Extract editing logic to useTaskEdit hook (56 lines) - Create TaskTooltip component for edit hint (32 lines) - Create TaskActions component for delete/complete buttons (84 lines) - Reduce TaskItem from 269 to 135 lines (-50%) - Improve testability and maintainability Addresses PR sizebay#52 feedback on component complexity

- Introduce Playwright E2E tests covering main user flows - Remove implementation-detail assertions from unit tests - Remove redundant and mocked integration tests - Focus unit tests on behavior verification - Configure jest to ignore E2E paths - Adjust coverage thresholds to behavior-oriented strategy

breno5g · 2026-03-12T12:32:30Z

+
+test.describe("Todo App - User Flows", () => {
+  test.beforeEach(async ({ page }) => {
+    await page.goto("/");


Given that we have a before-each statement, passing this in each test is redundant.

breno5g · 2026-03-12T12:34:28Z

+    await expect(progressBar).toHaveAttribute("aria-valuenow", "0");
+
+    // Complete task
+    await page.getByText("Buy bread").click();


Instead of using mutable attributes, tests should be guided by elements or, in more specific cases, data attributes.

breno5g · 2026-03-12T12:35:44Z

+
+/**
+ * Helper to type text into an input - simulates real user typing
+ * Playwright's fill() doesn't trigger React onChange correctly


This is a strong and potentially incorrect statement. Playwright's fill() triggers input events and works correctly with React in modern versions. The helper may be unnecessary and adds an artificial 30ms delay per character, unnecessarily slowing down the suite. It should be documented with a reference or issue.

breno5g · 2026-03-12T12:37:56Z

+    await page.goto("/");
+    await page.evaluate(() => localStorage.clear());
+    await page.reload();


Given that we already have a goto, the reload becomes redundant in this scenario.
By changing the order of the calls, we can eliminate one of them.

breno5g · 2026-03-12T12:39:01Z

+    await addButton.click();
+
+    // Search
+    await typeIntoInput(page.getByPlaceholder("search items"), "buy");


Again, a problem with mutable selectors.

If the application were in another language, for example, this element would not appear in this way.

breno5g · 2026-03-12T12:39:44Z

+    await page.getByLabel("Delete task: To delete").click();
+
+    // Task gone
+    await expect(page.getByText("To delete")).not.toBeVisible();


If the element is in the DOM but hidden (e.g., display: none), the test passes even if the item hasn't been removed. A more precise assertion would be not.toBeAttached() or toHaveCount(0) to guarantee actual removal from the DOM.

breno5g · 2026-03-12T12:42:26Z

+import { useTaskContext } from "./hooks/useTaskContext";
+
+function TodoApp() {
+  const { tasks, addTask, toggleTask, updateTask, deleteTask, filter, setFilter, searchValue, setSearchValue, filteredTasks, isLoading } = useTaskContext();


TodoApp is consuming 11 context values at once. This violates the single responsibility principle—the component knows everything. Ideally, each child component would only consume the slice of context it needs. This also ensures that any change to the context re-renders the entire TodoApp, even if the affected child is only FilterBar.

breno5g · 2026-03-12T12:44:57Z

+function TodoApp() {
+  const { tasks, addTask, toggleTask, updateTask, deleteTask, filter, setFilter, searchValue, setSearchValue, filteredTasks, isLoading } = useTaskContext();
+
+  const completedCount = tasks.filter(task => task.completed).length;


This value is calculated in TodoApp but should be in useTaskContext along with filteredTasks. filteredTasks is already a derived value in the context — completedCount should have the same treatment to avoid scattered logic. It also iterates tasks on a third pass in addition to those that the context already does.
And we also have the problem of generating new values with each render.

breno5g · 2026-03-12T12:45:20Z

+  return (
+    <div className="min-h-screen bg-[#555555] flex items-center justify-center p-4">
+      <Modal>
+        <div className="w-[309px] h-auto md:w-[680px] md:h-[495px]">


The values 309px and 680px appear twice, coupling the inner and outer layouts. If the breakpoint changes, it needs to be altered in two places. These should be extracted as CSS variables, Tailwind tokens, or a layout wrapper component.

breno5g · 2026-03-12T12:46:49Z

+            searchValue={searchValue}
+            onSearchChange={setSearchValue}
+          />
+          <div className="w-[309px] h-auto md:w-[680px] md:h-[280px]">


And, speaking purely from a development standpoint, defining the size of the children separately is a terrible practice, as it forces you to do all the responsiveness manually, whereas with the flex display and relative measurements, this is resolved automatically.

breno5g · 2026-03-12T12:47:10Z

+  const completedCount = tasks.filter(task => task.completed).length;
+
+  return (
+    <div className="min-h-screen bg-[#555555] flex items-center justify-center p-4">


Arbitrary color outside the design system. It should be a token (e.g., bg-surface or bg-zinc-600), especially if the project has a theme.

breno5g · 2026-03-12T12:48:07Z

+}
+
+export default App;
+


We have a blank line at the end of the file, which shouldn't happen with a properly configured linter.

breno5g · 2026-03-12T12:50:25Z

+          key={task.id}
+          task={task}
+          isSelected={selectedTaskId === task.id}
+          onSelect={() => setSelectedTaskId(selectedTaskId === task.id ? null : task.id)}


A new function is created for each item, on each render. Inside a .map(), the impact is multiplied by the number of tasks. If TaskItem is memoized (or becomes memoized), this breaks memoization completely. It should be a stable handler, e.g., useCallback with the id as an argument.

breno5g · 2026-03-12T12:52:24Z

+  const prevFilterRef = useRef<TaskFilter | undefined>(currentFilter);
+  const prevSearchRef = useRef<string | undefined>(searchValue);


This pattern manually simulates prevProps. The problem: useEffect already reacts to changes in dependencies—the internal conditional check and refs are redundant. The same effect could be achieved with:

useEffect(() => { setSelectedTaskId(null); }, [currentFilter, searchValue]);

breno5g · 2026-03-12T12:54:13Z

+
+  // Mensagem para busca sem resultados
+  if (tasks.length === 0 && searchValue && searchValue.trim()) {
+    return (


Three if blocks with repetitive JSX and duplicated dimensions (w-[309px] md:w-[680px] h-[19px]). Besides the duplication, the logic for which message to display is mixed with the JSX rendering. It should be extracted into an EmptyState component or at least a helper function.

Lines 52-93

breno5g · 2026-03-12T12:54:37Z

+  // Mensagem para busca sem resultados
+  if (tasks.length === 0 && searchValue && searchValue.trim()) {
+    return (
+      <div className="w-[309px] md:w-[680px] h-[19px] text-left">


The values 309px, 680px, 19px, 14px, #848484, and #3d8a94 appear in multiple places in the file and likely in other files in the project. Without tokens, any visual adjustment becomes a manual find-replace, risking inconsistency.

breno5g · 2026-03-12T12:54:54Z

+    }
+  }, [currentFilter, searchValue]);
+
+  // Mensagem para busca sem resultados


The rest of the code (JSX, strings, props) is in English. Mixed-language comments make maintenance difficult for mixed teams and are inconsistent with the file standard.

breno5g · 2026-03-12T12:55:15Z

+  }
+
+  if (tasks.length === 0) {
+    return <div className="w-[309px] md:w-[680px] h-[19px]"></div>;


An empty element with a fixed height to "reserve space" is fragile and lacks semantics. If the layout changes, this phantom spacing is difficult to track. It should be null or a true empty state component.

breno5g · 2026-03-12T12:57:06Z

+  /* Shared settings for all the projects below */
+  use: {
+    /* Base URL to use in actions like `await page.goto('/')`. */
+    baseURL: "http://localhost:5174",


Here, a port is set, but in the project settings it's not fixed, so on my machine, for example, it starts on a port with the lowest value; therefore, the tests will not run.

breno5g

When loading the page for the first time (ctrl + shift + r) we can see all the text on the page flashing with a different font.
The src/constants/theme.ts file defines COLORS, DIMENSIONS, and ANIMATION as TypeScript constants. However, the components use hardcoded values in Tailwind (text-[#848484], w-[309px], bg-[#555555]). The two sources are not connected—any visual adjustments need to be made in two places, with a real risk of inconsistency. The tokens exist but are not used.
The selection state of which task is active (selectedTaskId) resides in TaskList and is propagated to each TaskItem via isSelected + onSelect. This means that TaskList re-renders all items when any one is selected. The selection state is local to an item—it should be managed with an approach that isolates the re-rendering (e.g., state in the TaskItem itself or a dedicated selection Context).
In src/components/TaskItem/index.tsx, the tooltip position is calculated via getBoundingClientRect() and stored in state. This breaks down into: list scroll (absolute vs. relative coordinates to scroll), browser zoom, and any reflow caused by resize. The use of portal is correct, but the position calculation should use IntersectionObserver or CSS anchoring instead of absolute coordinates.
The function uses Date.now() (1ms resolution) + 7 random characters. In quick sequential creations (e.g., pasting multiple tasks, automation), collisions are possible. For a local app with localStorage, the risk is low, but the function lacks documentation of its limitations—and if the logic migrates to a backend, duplicate IDs become a data integrity problem. crypto.randomUUID() is available in all modern browsers and solves this at no cost.

AllanMicuanski and others added 29 commits September 30, 2024 23:25

feat:added header/style

0e9fe06

feat:added ProgressBar/style

e76d0de

fix:styles/header/progress

7775645

feat:Addeed Filter/Task/Search components

26842f2

feat:progressBar

d1d19bd

Merge pull request #1 from AllanMicuanski/dev

1c461b3

progressBar

fix:position filterContainer

6078f6f

fix:filter buttons

eb046c6

feat:searchBar icons

586fb84

fix:style container filter

16e1e3e

addTask styles

21ba316

feat:global styles adjust

d489ca8

Merge pull request #2 from AllanMicuanski/dev

af7032a

lesgo

fix: task bar styles

eef3f19

feat: added hiden cta task-bar

49ad9b9

fix:complete/deleted cta

d812279

fix: tooltip

6c8cb47

fix: edit task

82afc7c

fix: edit task

0d219c0

Merge pull request #3 from AllanMicuanski/fix/task-bar

4eb52e2

Fix/task bar

fix: responsivenes

022b11d

fix: some more styles

f001f47

fix: app width mobile

3fe1605

Merge pull request #4 from AllanMicuanski/fix/responsive

4fd2067

fix: responsivenes

feat: add pnpm | mise

7121012

Merge pull request #5 from AllanMicuanski/chore/migration

be18dea

feat: add pnpm | mise

docs: added readme

43dd4eb

Merge pull request #6 from AllanMicuanski/docs/readme

965eb4f

docs: added readme

feat: new implementation of frontend challenge

59d8bf0

AllanMicuanski requested a review from breno5g March 6, 2026 18:50

AllanMicuanski and others added 9 commits March 7, 2026 15:33

docs: update README with completed improvements

a3f0542

- Added clsx to technologies section - Highlighted design tokens implementation - Updated TaskForm icon color to match primary brand color (#4DA6B3)

refactor: fix architecture anti-patterns

456a1ab

App.tsx: remove useState callback anti-pattern TaskList: use useRef instead of callback for deselection SearchBar: delete unused component (-73 lines) Header: fix month to uppercase Addresses PR sizebay#52 feedback point #1

docs: update README

f1bbe59

fix: Delete coverage directory

06ed590

AllanMicuanski requested a review from breno5g March 7, 2026 23:24

breno5g reviewed Mar 12, 2026

View reviewed changes

		const prevFilterRef = useRef<TaskFilter \| undefined>(currentFilter);
		const prevSearchRef = useRef<string \| undefined>(searchValue);

Uh oh!

Conversation

AllanMicuanski commented Mar 6, 2026

Uh oh!

AllanMicuanski commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breno5g left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AllanMicuanski commented Mar 7, 2026 •

edited

Loading