-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(config): upgrade angular-bootstrap from 0.13 to 1.0 #1143
Conversation
@trendzetter you should review our CONTRIBUTING documentation since it seems you need to squash your commits and change the commit text to our standards. |
@lirantal I'm a bit new to this, I tried to "squash" my commits and format my text according to the docs. Am I doing it better now? |
@trendzetter This is generally how I squash my commits down to 1 commit. Also, I see you have merge commits here. It's better to not use merge for contributing, as it will sometimes pull in other changes that have already been merged upstream; thus, making it look like your made the changes & there can be conflicts when merging the PR. Use rebase instead, as described here http://blog.bigbinary.com/2013/09/13/how-to-keep-your-fork-uptodate.html
This took some time for me to get comfortable with, but it's really not as scary as it appeared to me at first :) The articles I shared really helped me a lot. |
I am grateful for you helpful attitude. |
LGTM |
@mleanos did you test this as well, or just the change for the commit message? |
The squash looks good, and the diffs look fine to me. However, I haven't tested. I wanted to, but I don't have time today. @trendzetter Have you done a quick once over, and made sure nothing on the UI was adversely affected by this? |
@trendzetter I haven't worked with ui-bootstrap after 0.13, so I'm not sure if it will change the CSS and look in addition to the directives and such as well. Have you tested the app with these changes? |
@lirantal Yes. All seemed to work after fixing the menu. After more systematic screening the code I have found more directives that need updating where I wasn't aware off. I will update my pr. |
I have updated my commit based on a carefully checking all ui-bootstrap components. It took me some wrestling with git. I will take a break and give it some more extensive testing. |
Further testing revealed no regressions. This should be encouraging to those heavily invested in angular-bootstrap. |
@ilanbiala They renamed their directives. Nothing is deprecated. Some directives are extended with additional options since 0.13. The migration guide sums the breaking changes up: |
SGTM, haven't tested, but based on the docs, seems fine. |
I ran through all the tests, and manually tested this branch. Everything looks good to me, with the exception of the confirm dialog not implemented in the article client controller. After looking at the code, I realized that this branch isn't current with master. @trendzetter Can you rebase with the upstream master branch, and push? Normally, it wouldn't be much of an issue, since the changes would just be merged on top of master. However, these changes have a wide reach over the UI. I'd like to test with the most up to date code-base, to be sure we don't introduce any bugs/issues. |
Many issues arise when using angular-bootstrap 0.13. Migration to 1.0 is straightforward, offers a more stable interface, extra directives and fixes many bugs. Migration guide: https://github.com/angular-ui/bootstrap/wiki/Migration-guide-for-prefixes Fixes #1142 Fixes #1131
I've synced with the upstream master. The confirm dialog for delete is correctly displayed. I am not seeing any new issues which is expected as the articles module is not using any of the renamed angular-bootstrap directives. |
@trendzetter LGTM, just maybe change the scope of the commit message? @codydaig suggestions? |
@ilanbiala I'm going to pull this branch down tomorrow and test. I'll ping back tomorrow with any comments. |
@codydaig Did you find any issues that I need to look in to? |
@codydaig is this good to merge? |
feat(config): upgrade angular-bootstrap from 0.13 to 1.0
Moving from 0.13 solves many the many issues with this version, provides a more stable interface.
Migration guide: https://github.com/angular-ui/bootstrap/wiki/Migration-guide-for-prefixes
Closes #1142
Closes #1131