Custom alert style #167
Custom alert style #167
Conversation
phalkunz
force-pushed
the
pulls/1.0/create-success-msg
branch
from
efd87d6
to
940092b
Compare
|
|
||
| getSavedIcon() { | ||
| // In case this is specified directly | ||
| return this.props.savedIcon || this.props.data.savedIcon || null; |
There was a problem hiding this comment.
Why are all these fields in two places? They should either be on this.props or this.props.data.
There was a problem hiding this comment.
It looks like there's a bug that you're working around rather than fixing.
| if (savedClasses && this.props.changed) { | ||
| savedClasses.split(' ').forEach((cl) => { | ||
| buttonClasses[`btn-${cl}`] = true; | ||
| }); |
There was a problem hiding this comment.
can the classes have the btn- prefix when declared/added rather than magically added here?
Or perhaps it's more appropriate to call this savedButtonClass...
| @@ -72,6 +86,17 @@ class FormAction extends SilverStripeComponent { | |||
| } | |||
|
|
|||
| /** | |||
| * @return {boolean} | |||
| */ | |||
| isConstructive() { | |||
There was a problem hiding this comment.
I think isPrimary() or isPrimaryType() is a better name, it took me a while to work out what "constructive" meant and I doubt I'll remember a month later :)
| const changed = !!( | ||
| state.unsavedForms && | ||
| state.unsavedForms.find(form => ownProps.identifier === form.name) | ||
| ); |
There was a problem hiding this comment.
Could we use the isDirty() API provided by redux-forms?
isDirty(form.name, getFormState)(state);
| // Action text when there's no changes detected in the form | ||
| savedTitle: React.PropTypes.string, | ||
| savedIcon: React.PropTypes.string, | ||
| savedClasses: React.PropTypes.string, |
There was a problem hiding this comment.
missing definition in the data prop
| type: React.PropTypes.string, | ||
| loading: React.PropTypes.bool, | ||
| icon: React.PropTypes.string, | ||
| disabled: React.PropTypes.bool, | ||
| changed: React.PropTypes.bool, |
There was a problem hiding this comment.
this should be named formChanged I think, not to be confused with this actual component changing somehow
phalkunz
force-pushed
the
pulls/1.0/create-success-msg
branch
from
940092b
to
dfc698b
Compare
unclecheese
force-pushed
the
pulls/1.0/create-success-msg
branch
2 times, most recently
from
1db560b
to
12f600e
Compare
unclecheese
force-pushed
the
pulls/1.0/create-success-msg
branch
from
a22e6b7
to
e1d716e
Compare
client/src/boot/transforms.js
Outdated
| }); | ||
| return { | ||
| ...field, | ||
| title: isPristine ? field.data.pristineTitle : field.data.dirtyTitle, |
There was a problem hiding this comment.
What if pristineTitle and dirtyTitle aren't set? FormField.php doesn't set these by default either.
There was a problem hiding this comment.
You could get rid of dirtyTitle and just have title (for dirty / normal) and optional pristineTitle
There was a problem hiding this comment.
Have updated this to have fallback value
client/src/boot/index.js
Outdated
| @@ -8,6 +8,7 @@ import { setConfig } from 'state/config/ConfigActions'; | |||
| import registerComponents from 'boot/registerComponents'; | |||
| import registerReducers from 'boot/registerReducers'; | |||
| import applyDevtools from 'boot/applyDevtools'; | |||
| import transforms from './transforms'; | |||
There was a problem hiding this comment.
import from boot/transforms would be following existing conventions, could this be named applyTransforms
client/src/boot/transforms.js
Outdated
| return { | ||
| ...field, | ||
| title: (isPristine ? field.data.pristineTitle : field.data.dirtyTitle) || field.title, | ||
| icon: (isPristine ? field.data.pristineIcon : field.data.dirtyIcon) || field.icon, |
There was a problem hiding this comment.
could we move the isPristine ? x : y parts to customTitle/Icon constants?
Issue: silverstripe/silverstripe-campaign-admin#16