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

Reset placeholder display attribute instead of changing it to block #631

Merged
merged 1 commit into from Nov 8, 2015
Merged

Reset placeholder display attribute instead of changing it to block #631

merged 1 commit into from Nov 8, 2015

Conversation

wwsno
Copy link
Contributor

@wwsno wwsno commented Oct 22, 2015

Reset placeholder css display attribute, so it resets to whatever the placeholder class display attribute was set to.

In case someone is using a custom placeholder class this can unexpectedly change the css, for example when display is set to 'block-inline';

Pull request #525 added this behavior.

…which changed the original placeholder class
@Voles
Copy link
Member

Voles commented Nov 4, 2015

Thanks for this, @wwsno ! The PR looks good. Can you add a test for this behaviour, so we explicitly check the original display value is being used after the reset? Thanks in advance!

@Voles Voles added the reviewed label Nov 4, 2015
@Voles Voles mentioned this pull request Nov 4, 2015
@wwsno
Copy link
Contributor Author

wwsno commented Nov 6, 2015

What do you expect from a test for this? uiTreeNode seems completely untested and it seems rather non trivial to add a single test for this fix.

Voles pushed a commit that referenced this pull request Nov 8, 2015
Reset placeholder display attribute instead of changing it to block
@Voles Voles merged commit ff4bb34 into angular-ui-tree:master Nov 8, 2015
@Voles
Copy link
Member

Voles commented Nov 8, 2015

You're right, tests are important but it's not trivial to add them for this behaviour. There are other parts which are more important to get covered for now. Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants