Closed jeffgolenski closed 8 years ago
Changes look good to me in Jetpack, but we need to make sure that if any other dops products are using this the classname change won't break their styles.
Found another issue with these notices: https://github.com/Automattic/jetpack/issues/3945
@jeffgolenski The changes look good. As for IE10, we need to use a polyfill, I think.
There are definitely instances of notice__text
in Akismet's React UI that will probably need to be updated to use the new classnames.
@beaulebens Great. Tomorrow I'll scour through and make anymore changes I can find in my PR over at https://github.com/Automattic/akismet-react/pull/20 – do you know if any VP repos may be using this? I'm unfamiliar with Akismet and VP
I don't believe they're in place anywhere for VP.
@beaulebens Great! I went through https://github.com/Automattic/akismet-react/pull/20 and did another sweep. I think I found all the relevant changes that needed to be made there, but will need the akismet team to help test it out!
@jeffgolenski LGTM
Made a couple of small adjustments in regard to naming conventions of notices and icon position.
Changed the name of
.notice
to.dops-notice
since the general.notice
class name is used by core, and potentially other plugins, which could make it fairly susceptible to style bleeding.Fixing issues in: https://github.com/Automattic/jetpack/pull/3974 Related changes: https://github.com/Automattic/akismet-react/pull/20
Before:
After: