Skip to content

feat: change heading based on filter #195

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ function App(props) {
}

const tasksNoun = taskList.length !== 1 ? "tasks" : "task";
const headingText = `${taskList.length} ${tasksNoun} remaining`;
let headingText = `${taskList.length} ${tasksNoun}`;
if (filter === "Active") headingText += " remaining";
if (filter === "Completed") headingText += " completed";

Comment on lines +87 to 90

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @elle-l-em! I've reviewed your PR and the original documentation carefully. There's actually a semantic misunderstanding here worth discussing:

  1. Current Documentation is Correct
  • The heading X tasks remaining shows the total count (all tasks), this filter you put in doesn't make sense.
    • It's consistent with the UI (the counter appears above the filters)
    • "Remaining" here means "total existing", not "uncompleted"
    • Filter status is already visually indicated by the tabs themselves
  1. Suggested Improvements Instead
  • If we wanted to improve this, we could remove the word “remaining” as it explicitly refers to the total quantity:
  const headingText = taskList.length === 0 
  ? "No tasks found" 
  : `${taskList.length} ${tasksNoun}`;
  1. Alternative (Advanced)
  • For a more detailed status, we could show:
const remaining = taskList.filter(t => !t.completed).length;
const completed = taskList.length - remaining;

const headingText = taskList.length === 0
  ? "No tasks found"
  : `Total: ${taskList.length} | Active: ${remaining} | Completed: ${completed}`;
  • UI adjustments would be needed for space

Recommendation:
Let's either:
a) Keep the original (correct as documentation)
b) Implement the "0 tasks" edge case fix
c) Move the status details elsewhere in UI

What do you think? The current PR change actually introduces a misleading coupling between the counter and filters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elle-l-em - there's some comments here for you to have a look at. Would you like to come back to this? Thanks!

const listHeadingRef = useRef(null);
const prevTaskLength = usePrevious(tasks.length);
Expand Down