Closed Xananax closed 8 years ago
Xananax
commented
8 years ago By the way, I realize I can get away with
t.form.Form.i18n = {
optional: ''
, required: ''
, add: '➕'
, remove: '✖'
, up: '▲'
, down: '▼'
}But that's pretty limited in terms of making those buttons consistent with the rest of my theme.
minedeljkovic
commented
8 years ago What makes it really hard to do it in template only is that there is no way to properly differentiate between remove, up, down buttons (well you can hack with labels).
Maybe some improvement in List options would make it easier (maybe even enable styling without template):
var options = {
remove: {
className: 'someRemoveButtonClass'
},
up: {
template: someUpButtonTemplate
}
}or maybe to make it more inline with other simple components options:
var options = {
remove: {
attrs: {
className: 'someRemoveButtonClass'
},
disabled: true
},
up: {
template: someUpButtonTemplate
}
}A simple fix for styling would be to at least give the buttons default classes
const buttons = [];
if (!options.disableRemove) { buttons.push({ label: i18n.remove, click: this.removeItem.bind(this, i),classname:'btn-remove'}); }
if (!options.disableOrder) { buttons.push({ label: i18n.up, click: this.moveUpItem.bind(this, i),className:'btn-move-up' }); }
if (!options.disableOrder) { buttons.push({ label: i18n.down, click: this.moveDownItem.bind(this, i),className:'btn-move-down' }); }This would give some room to play around.
I realize this should not be the concern of the factory, and should be handled in the template, and I agree with the choice in principle, but in practice, it might be worth it to favour ease of use over purity. Currently, from the template, there is no way to know which button does what (other than by checking the label, which is a very brittle way). Maybe a custom btnType property that the template could use?
In the factory:
if (!options.disableRemove) { buttons.push({ label, click,btnType:'remove'}); }in the bootstrap template:
return bootstrap.getButton({
click: button.click,
key: i,
label: button.label,
className:(button.btnType?'btn-'+button.btnType:'')
});(not sure uvdom uses className, this is just pseudo-code).
This doesn't seem like a big change in architecture and would open the door for further customization. I'd be willing to submit a PR, but I'll wait for @gcanti to pitch in before making a move.
Maybe it would be even better if the template returned something like this:
<a role="button" className={`btn btn-${btnType}`}>
<span className={`btn-label btn-label-${btnType}`}>{label}</span>
</a>Having an <a/> and a nested <span/> would allow for all but the most complex stylings.
Xananax
commented
8 years ago As far as I can tell, here's a minimal implementation for a bootstrap-styled list factory:
import React,{Component,PropTypes} from 'react';
import t from 'tcomb-form';
import cx from 'classnames';
const noobj = {};
const {getComponent,List} = t.form;
class MyList extends List{
getItems() {
const { options, ctx } = this.props;
const auto = this.getAuto();
const i18n = this.getI18n();
const config = this.getConfig();
const templates = this.getTemplates();
const value = this.state.value;
const type = this.typeInfo.innerType.meta.type;
const Component = getComponent(type, options.item || noobj);
return value.map((value, i) => {
const buttons = {};
buttons.remove = (!options.disableRemove) && this.removeItem.bind(this, i);
buttons.moveUp = (!options.disableOrder) && this.moveUpItem.bind(this, i);
buttons.moveDown = (!options.disableOrder) && this.moveDownItem.bind(this, i);
return {
input: React.createElement(Component, {
ref: i,
type,
options: options.item || noobj,
value,
onChange: this.onItemChange.bind(this, i),
ctx: {
context: ctx.context,
uidGenerator: ctx.uidGenerator,
auto,
config,
i18n,
name: ctx.name ? `${ctx.name}[${i}]` : String(i),
templates,
path: ctx.path.concat(i)
}
}),
key: this.state.keys[i],
buttons
};
});
}
getLocals() {
const {options} = this.props;
const {value} = this.state;
const add = (!options.disableAdd) && this.addItem.bind(this);
const items = this.getItems();
const itemsHaveButtons = !(options.disableRemove && options.disableOrder);
return {
typeInfo: this.typeInfo,
path: this.props.ctx.path,
error: this.getError(),
hasError: this.hasError(),
label: this.getLabel(),
onChange: this.onChange.bind(this),
config: this.getConfig(),
value,
disabled: options.disabled,
help: options.help,
add,
itemsHaveButtons,
items,
className:options.className
};
}
getTemplate() {
return this.props.options.template || myListTemplate;
}
}
function myListTemplate(locals){
const children = [
(locals.help) && (<div className='alert-info' key="alert-info">{locals.help}</div>)
, (locals.error && locals.hasError) && (<div className='alert-danger' key="alert-danger">{locals.error}</div>)
, ...locals.items.map((!locals.itemsHaveButtons)?
((item) => (<div className='row' key={item.key}>
<div className='col-xs-12'>{item.input}</div>
</div>)) :
((item,i) => (<div className='row' key={item.key}>
<div className='col-sm-8 col-xs-6'>
{item.input}
</div>
<div className='col-sm-4 col-xs-6'>
<div className='btn-group'>
{(item.buttons.remove) && <button className='btn' onClick={item.buttons.remove}>remove</button>}
{(item.buttons.moveUp) && <button className='btn' onClick={item.buttons.moveUp}>up</button>}
{(item.buttons.moveDown) && <button className='btn' onClick={item.buttons.moveDown}>down</button>}
</div>
</div>
</div>))
)
, (locals.add) && (<div className='row' key='addBtn'>
<div className='col-lg-12'>
<div style={{marginBottom:15}}>
<button className='btn' onClick={locals.add}>add</button>
</div>
</div>
</div>)
];
return (<fieldset className={locals.className} disabled={locals.disabled}>
<legend>{locals.label}</legend>
{children}
</fieldset>)
}
export default MyList;...later:
{
fields:{
listField:{
factory:MyList
}
}
}Another, less idiomatic, but simpler possibility:
class MyList extends List{
render(){
const locals = this.getLocals()
const children = [
(locals.help) && (<div className='alert-info' key="alert-info">{locals.help}</div>)
, (locals.error && locals.hasError) && (<div className='alert-danger' key="alert-danger">{locals.error}</div>)
, ...locals.items.map((item,i) => (<div className='row' key={item.key}>
<div className='col-sm-8 col-xs-6'>
{item.input}
</div>
<div className='col-sm-4 col-xs-6'>
<div className='btn-group'>
<button className='btn' onClick={this.removeItem.bind(this,i)}>remove</button>
<button className='btn' onClick={this.moveUpItem.bind(this,i)}>up</button>
<button className='btn' onClick={this.moveDownItem.bind(this,i)}>down</button>
</div>
</div>
</div>))
, (<div className='row' key='addBtn'>
<div className='col-lg-12'>
<div style={{marginBottom:15}}>
<button className='btn' onClick={this.addItem.bind(thid)}>add</button>
</div>
</div>
</div>)
];
return (<fieldset className={locals.className} disabled={locals.disabled}>
<legend>{locals.label}</legend>
{children}
</fieldset>)
}
}Hope it provides a good starting point to whomever wants to add functionality. I'm not sure about the bootstrap classes, I just mentally parsed what's happening over at uvdom-bootstrap (I don't use bootstrap so I just added them for the example).
gcanti
commented
8 years ago Hi, sorry for the delay.
Thanks a lot for your suggestions, very interesting.
So, in order to customize the template, one would have to...
@Xananax currently no need for 1. and 3. just set the template option
function myListTemplate(locals) {
// you can use jsx here
return <div>myListTemplate...</div>;
}
const Type = t.struct({
tags: t.list(t.String)
});
const options = {
fields: {
tags: {
template: myListTemplate // <= local override
}
}
};What makes it really hard to do it in template only is that there is no way to properly differentiate between remove, up, down buttons
A simple fix for styling would be to at least give the buttons default classes
@minedeljkovic @Xananax I agree, that would give some room to basic styling, will add a type property to the button locals https://github.com/gcanti/tcomb-form/blob/master/src/components.js#L725
then the template could add proper style classes https://github.com/gcanti/tcomb-form/blob/master/src/templates/bootstrap/list.js#L58
It seems to me that it would be better if the buttons were their own components, so they could be skinned just like everything else:
Seems a great idea, what about something like this? (renderers factored out in the list template)
https://github.com/gcanti/tcomb-form/blob/master/src/templates/bootstrap/list.js
and then override like this
// override the row buttons...
t.form.Form.templates.list.renderRowButton = function (button) {
return <button onClick={button.click}>{button.label}</button>;
};
// ...or override the add buttons, and so on...
t.form.Form.templates.list.renderAddButton = function (button) {
return <button onClick={button.click}>{button.label}</button>;
};
const Type = t.struct({
tags: t.list(t.String)
});seems quite easy, what do you think?
minedeljkovic
commented
8 years ago I think the templates flexibility that you added are step in the right direction. It reminds me a lot of a concept (from godforsaken platform that I spent a lot time working on) of Template parts (http://www.jeff.wilcox.name/2010/04/template-part-tips/).
Increase in flexibility is obvious. You can change parts on an existing template globally or locally (just clone global template, change that little part that must be different in just that one place and set that new template through options).
Of course (as with everything) it takes skill to define "just enough" parts, not to make everything over complex, but the tools are here.
For me, nicely done!
gcanti
commented
8 years ago Just added a clone method to templates, now you can do local overrides:
Global override
function renderAddButton(locals) {
return <button onClick={locals.add.click}>{locals.add.label}</button>;
}
// every list is affected
t.form.Form.templates.list.renderAddButton = renderAddButton;
const Type = t.struct({
a: t.list(t.String),
b: t.list(t.String)
});Local override
function renderAddButton(locals) {
return <button onClick={locals.add.click}>{locals.add.label}</button>;
}
const myList = t.form.Form.templates.list.clone();
myList.renderAddButton = renderAddButton;
const Type = t.struct({
a: t.list(t.String),
b: t.list(t.String)
});
const options = {
fields: {
b: {
template: myList // <= only list `b` is affected, list `a` use defaults
}
}
};Do you find that deep clone is necessary @gcanti?
The problem I see with the actual implementation (i.e. attaching the render* functions to the function list) is that list will retain the references to the render* functions captured during its definition (and so will do the render* functions as well), thus if list is not redefined list.renderSomething = myRenderSomething doesn't work (is ignored if my tests are correct).
Do you have suggestions for an alternative implementation?
minedeljkovic
commented
8 years ago Yes, I was completely off with my view of clone, forgetting that templates are of course functions. I was quick to fix my comment, but you were quicker. :-) There are of course some solutions for function cloning (http://stackoverflow.com/a/11230005), but it is not very elegant (which can be a sign of need for a different approach). I will be thinking on this.
minedeljkovic
commented
8 years ago Slightly modified version maybe?
list template:
function create(overrides = {}) {
function list(locals) {
...
}
list.extend = create;
list.renderAddButton = overrides.renderAddButton || function (locals) {
// default implementation
};
return list;
}
export default create();function renderAddButton(locals) {
return <button onClick={locals.add.click}>{locals.add.label}</button>;
}
const myList = t.form.Form.templates.list.extend({ renderAddButton });
...@minedeljkovic great idea, one last problem though
function myRenderAddButton(locals) {
return <button onClick={locals.add.click}>{locals.add.label}</button>;
}
console.log(t.form.Form.templates.list.renderAddButton.name); // => renderAddButton (default) OK!
const myList1 = t.form.Form.templates.list.extend({renderAddButton: myRenderAddButton});
console.log(myList1.renderAddButton.name); // => myRenderAddButton OK!
const myList2 = myList1.extend();
console.log(myList2.renderAddButton.name); // => renderAddButton OUCH!modifying list.extend
function create(overrides = {}) {
function list(locals) {
...
}
list.renderAddButton = overrides.renderAddButton || function (locals) {
// default implementation
};
list.extend = function (overrides = {}) {
return create({...list, ...overrides}); // keep previous overrides
};
return list;
}
export default create();As a (beneficial?) side effect, you could even modify the overrides after the extend call
function myRenderAddButton(locals) {
return <button onClick={locals.add.click}>{locals.add.label}</button>;
}
const myList1 = t.form.Form.templates.list.extend();
myList1.renderAddButton = myRenderAddButton; // this now works well with extend()
const myList2 = t.form.Form.templates.list.extend({
renderAddButton: myRenderAddButton
});
console.log('default renderer', t.form.Form.templates.list.renderAddButton.name); // => renderAddButton (default) OK!
console.log('myList1', myList1.renderAddButton.name); // => myRenderAddButton OK!
console.log('myList1.extend()', myList1.extend().renderAddButton.name); // => myRenderAddButton OK!
console.log('myList2', myList2.renderAddButton.name); // => myRenderAddButton OK!
console.log('myList2.extend()', myList2.extend().renderAddButton.name); // => myRenderAddButton OK!One minor pick: not sure about the name, extend seems too OOPish... I'd keep clone
minedeljkovic
commented
8 years ago Yes, @gcanti great fix!
One minor pick: not sure about the name, extend seems too OOPish... I'd keep clone
I don't have preference towards extend (it even bothers me too), so of course.
I am even bothered with overrides. (I was thinking of calling it template, but a ui template's function template, ugh... config is of course vague..)
gcanti
commented
8 years ago As a (beneficial?) side effect...
Mhhh it's tricky... after additional tests there are some failing corner cases. I'd end up with this implementation
function create(overrides = {}) {
...
list.clone = function clone(newOverrides = {}) {
return create({...overrides, ...newOverrides});
};
return list;
}and discourage mutability
// DON'T DO THIS
const myList1 = t.form.Form.templates.list.clone();
myList1.renderAddButton = myRenderAddButton;
// do this instead
const myList1 = t.form.Form.templates.list.clone({
renderAddButton: myRenderAddButton
});moreover having only one way to get new templates seems a better practice.
I'm having a hard time wrapping my head around the best way to customize the those buttons.
The way I see it, currently, if I wanted to replace them with icons (respectively
✖,▲and▼), the process is quite involved.I tried following up the codepath where the buttons are created (for bootstrap templates) and it seems to me the following snippets are the important ones:
In
components.js, in thegetItems()method of theListcomponent, the buttons options get createdIn
templates/bootstrap.js, in thelisttemplate, the buttons get created:Then in the
getButton.jsfile of theuvdom-bootstrapmodule, the button actually gets instantiated.So, in order to customize the template, one would have to:
ListComponentlisttemplateuvdomanduvdom-boostrap(arguably not very hard, but still a slight additional learning curve).It's a complex process because both the List component and the list template make use of a bunch of helper functions which are not always exposed. They are also pretty complex (in other words, easy to break when the main library changes something).
Alternatively, one could create a factory that reimplements
onItemChange,removeItem,moveUpItem, andmoveDownItem, but you'd also have to reimplementvalidate, errors handling, and so on (so basically actually reimplementing theListcomponent almost in it's entirety).It seems to me that it would be better if the buttons were their own components, so they could be skinned just like everything else:
or:
Am I missing something? Is there an easier way to accomplish this that I'm not seeing?