-
Notifications
You must be signed in to change notification settings - Fork 75
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
TodoMVC Angular Bug Fixes #312
Conversation
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.
LGTM
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.
LGTM
@flashdesignory Would you mind rebuilding the angular complex version too? |
I just have to run |
I also run |
yup, did that as well. should be all in this pr now. Please let me know if I forgot anything. |
Do we have before/after numbers on browsers? |
The css bug fixes address user interaction, but don't change the test steps behavior. |
app-todo-item:last-child { | ||
border-bottom: none; | ||
} |
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.
Interestingly, :last-child can be costly, and this might explain why all browsers see a slight increase in their runtime. But I don't mind keeping it as it's very likely that this is used this way in real-world websites.
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.
Sorry for the delay, this looks good to me.
this slipped through somehow, but the dynamic classes of the todo items wasn't applied correctly.
Additionally, I needed to override some css styles to ensure the custom component don't interfere with display.
@kara