-
Notifications
You must be signed in to change notification settings - Fork 16
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
ENH CMS 6 compatibility #346
ENH CMS 6 compatibility #346
Conversation
dcbe5be
to
7391e6f
Compare
b2062e7
to
a2826ba
Compare
@@ -1,59 +0,0 @@ | |||
/* global jest, jasmine, describe, it, expect, beforeEach */ | |||
|
|||
import configureMockStore from 'redux-mock-store'; |
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.
This test was removed because 'redux-mock-store' wasn't compatible with latest version of redux. There was a link in the failed test to a page where the maintainers or redux recommend testing with react-testing-library instead. Since that kind of acts like a quasi behat test, and the fact we already have behat tests for campaign admin, I think we can safety just delete this test without a replacement
a2826ba
to
fca2c46
Compare
fca2c46
to
5e7ba3e
Compare
5e7ba3e
to
da63674
Compare
da63674
to
ecab072
Compare
5aa1456
to
5ccd01c
Compare
b2d8424
to
9d4cfab
Compare
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.
Tested locally - functionality all seems to be working as expected.
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.
Just noticed the draft
label displayed next to records in the campaigns doesn't have the correct styling - it's likely not using the new status badge API to get the markup for those.
9d4cfab
to
02cbef4
Compare
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.
Just the one change - sorry, should have noticed it last time
Also please respond to #346 (review) |
02cbef4
to
b61b41f
Compare
b61b41f
to
a6bdde7
Compare
Have updated styling |
a6bdde7
to
9836a98
Compare
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
Issue https://github.com/silverstripeltd/product-issues/issues/895
Requirement is in framework so we don't need to include it here as well
It's also the CMS 5 version of the plugin so means that campaign-admin currently isn't installable with CMS 6