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.

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

feat(page: detail): can add tags to item #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tirelyl
Copy link

@tirelyl tirelyl commented Sep 23, 2018

No description provided.

@wzhudev
Copy link
Member

wzhudev commented Sep 25, 2018

When I ran this application locally it threw errors.

image

Please make sure your changes are compatible with old data.

@tirelyl
Copy link
Author

tirelyl commented Sep 25, 2018

ok i'll check it @wendzhue

@tirelyl
Copy link
Author

tirelyl commented Oct 2, 2018

I'm busy recentlt, and check it on holiday online demo https://tirelyl.github.io/today-ng-steps/

Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

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

Your code is nice but you should take user experience into consideration. We need tag not just to put them there, but to search todos by tags. Feel free to modify the left-control's outlook if you feel necessary.

@@ -11,6 +11,28 @@
</div>
<label nz-checkbox (click)="$event.stopPropagation()" [(ngModel)]="currentTodo.completedFlag"></label>
<span>标记完成</span>
<nz-divider nzText="标签" nzOrientation="left"></nz-divider>
Copy link
Member

Choose a reason for hiding this comment

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

Tag has no superior priority to title and date. It should be placed after date and before description.

*ngIf="!addTagInputVisible"
class="editable-tag"
(click)="showAddTagInput()">
<i class="anticon anticon-plus"></i> New Tag
Copy link
Member

Choose a reason for hiding this comment

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

Use Chinese for now. i18n would be introduced later.

@wzhudev wzhudev added the 👨 User Exp More consideration about UE is needed label Oct 9, 2018
@haven110
Copy link

haven110 commented Sep 3, 2019

..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👨 User Exp More consideration about UE is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants