Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Issue 10441 ToDo Improvement #13979

Merged
merged 2 commits into from Mar 12, 2020
Merged

Issue 10441 ToDo Improvement #13979

merged 2 commits into from Mar 12, 2020

Conversation

majordwarf
Copy link
Collaborator

Partially fixes: #10441

Testing Plan:

  • Added extra field for taking user input for task description.
  • Task name renders bold.
  • Added Index numbers to keep track of the order of creation of tasks.

GIFs or Screenshots:
image

Issue

  • When I add a task, the task name field resets but the description field stays the same containing the text.

timabbott
timabbott reviewed Feb 21, 2020
View reviewed changes
static/templates/widgets/todo_widget_tasks.hbs Outdated
@@ -3,14 +3,14 @@
<li>
<button class="task" data-key="{{ key }}">
</button>
<span class="task">{{ task }}</span>
<span class="task"><b>{{ task }}</b></span>
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We generally use the modern strong and em tags, not b/i.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I'll change that. Should I add a new commit and push or squash all the commits abd push? As the documentation reads that solving of a particular issue should belong to a single commit.

Copy link
Member

Choose a reason for hiding this comment

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

@majordwarf You should add it to the 1st commit. You can do it in a number of ways. Rebasing or git commit --amend can both help with that.

@vinitS101
Copy link
Member

vinitS101 commented Feb 21, 2020 •
edited

I tried testing this and I came across an issue.

todo

The description field does not get cleared on clicking Submit.

@majordwarf
Copy link
Collaborator Author

@vinitS101 Yeah, I've mentioned that in the PR itself as an issue I'm facing. Can you guide me if you know what is causing that?

@vinitS101
Copy link
Member

vinitS101 commented Feb 22, 2020 •
edited

Check static\js\todo_widget.js, you will find this:

elem.find(".add-task").val('').focus();

You need to do something similar for .add-desc. This sets their value to null.

@majordwarf
todo_list: Render task name in bold text.
ba92b2a
@majordwarf
todo_list: Add task description and index numbers.
83122f7 
The user can pass description along with the task name by splitting the input string with hyphen.

Eg: Task Title - Task Description
todo_list: Add index numbers to task.
@majordwarf
Copy link
Collaborator Author

@vinitS101 @timabbott I guess everything is fixed now. 🤔

@vinitS101
Copy link
Member

LGTM.

@majordwarf majordwarf changed the title [WIP] Issue 10441 ToDo Improvement Issue 10441 ToDo Improvement Feb 24, 2020
@timabbott
Copy link
Sponsor Member

@showell can you give this a quick look and merge if you're happy with it?

I agree with the original poster of #10441 that ultimately what we actually want to do is support GitHub's TODO list format in our markdown syntax, but I'm fine with merging this tweak to the existing feature.

@showell showell merged commit 68dcdcd into zulip:master Mar 12, 2020
@showell
Copy link
Contributor

showell commented Mar 12, 2020

Merged! I am not super familiar with the todo widget, so I've never noticed its strange numbering issues. (We generally don't want zero-based indexes for user-facing things, and the indexes should sort.) But I think those quirks preceded your change.

Sorry it look so long to review this! Thanks for working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timabbott timabbott timabbott left review comments

@vinitS101 vinitS101 vinitS101 left review comments

Assignees
No one assigned
Labels
size: S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Todo feature improvement
5 participants
@majordwarf @vinitS101 @timabbott @showell @zulipbot