-
Notifications
You must be signed in to change notification settings - Fork 297
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
base: main
Are you sure you want to change the base?
Conversation
It looks like this is your first pull request. 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you. I think that's a good improvement. Would you be able to also update the doc here?
We can add a new section about making the heading consistent, showing the code addition here, then update the final code block at the end of the article. Does that make sense?
let headingText = `${taskList.length} ${tasksNoun}`; | ||
if (filter === "Active") headingText += " remaining"; | ||
if (filter === "Completed") headingText += " completed"; | ||
|
There was a problem hiding this comment.
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:
- 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
- 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}`;
- 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.
There was a problem hiding this comment.
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!
Description
Change the heading text so that it reflects the current tab. Previously, all tabs said "remaining", now they say:
Motivation
I started using this and was confused that when I went to my completed tasks, it said those tasks were still "remaining". All the tab filters say "remaining" but I think of "remaining" tasks as the tasks still left to do, not the completed ones. Now the header text appropriately reflects the active filter.
Additional details
None
Related issues and pull requests
None