search

vitalets / angular-xeditable

Edit in place for AngularJS
http://vitalets.github.io/angular-xeditable
MIT License
1.91k stars 403 forks source link

Added possibility to set icon_set via attribute #564

Closed fredrikw closed 7 years ago

fredrikw commented 7 years ago

Also fixed icon_set value if theme is set via the attribute.

The theme_name detection could be made more pretty, but this should work...

ckosloski commented 7 years ago

Can you add something to the documentation to show that this is available? Do all the tests pass?

fredrikw commented 7 years ago

I'm sorry, but how do I run the tests most easily? I couldn't find any instructions and when I loaded the Dev Test Runner at /test/e2e/dev-test.html none of the tests were working so I think I am missing something...

fredrikw commented 7 years ago

I think I found out that the reason for failing tests may be that the docs task failed for me while building, I'm looking at that now...

fredrikw commented 7 years ago

OK, I have now managed to get the tests running and all current tests pass. However, I am a bit at a loss when it comes to defining new tests and adding docs. One part of the PR is that before, when having no default theme (from the app.run function) the button classes where never set when a theme was added via editable-theme. This is not visible in the tests currently since they are all run with a default theme of bs3. However, if I comment out editableOptions.theme = 'bs3'; from app.js and change the dev-theme test to specify editable-themes = 'bs3' on any link I get buttons like in the attachment. I don't see an easy way of testing for this though, but it might be due to my limited experience with both jsdoc and the test runners... The other part, I can add both doc and tests for, I will update the PR shortly...

ckosloski commented 7 years ago

Couldn't you check the css classes applied to a specific element if you are looking for a specific theme that was applied?

Also, make sure to squash your commits.

fredrikw commented 7 years ago

The problem is that when editableOptions.theme is set, the bug won't show up since it requires a non-existing editableOptions.theme and editable-theme set to something else than default. Then self.icon_set = editableOptions.icon_set === 'default' ? editableIcons.default[editableOptions.theme] : editableIcons.external[editableOptions.icon_set]; will evaluate to undefined instead of editableIcons['bs3'] (since editableOptions.theme is still set to 'default') and hence give a situation like the one that I forgot to attach to the last comment... dev

And this, I don't know how to test for since it requires unsetting editableOptions.theme that is set in the app.js file for the full test suite...

ckosloski commented 7 years ago

Yeah, not sure you can test it with the tests. Maybe just be clear in the documentation what needs to be set.

fredrikw commented 7 years ago

I think I managed to get everything done now... There are tests for both the fix when the wrong icon-set was used on items with editable-theme set (tested with editable-theme="bs2" that gives bs3-icons on 0.5.0) and the addition with editable-icon-set. I also added a short sentence to the docs describing editable-icon-set. There were some changes needed to make the tests work, the dev-theme/tests.js was never run and even though the css files aren't needed for the tests, I think it is nice to have the possibility to test manually as well and see that it looks like expected.

ckosloski commented 7 years ago

Looks good to me. @eugef please take a look.

eugef commented 7 years ago

Looks good for me as well, @fredrikw thanks for you PR!