Automatically call transferAttributes as needed. #133
Conversation
- split out XHPBaseHTMLHelpers to just implement the ID and class helpers without transferAttributes - added tests - XHPHelpers should probably be renamed to XHPHTMLHelpers in XHP-Lib 3, and the bulk of the transfer/copyattribtues functiontionality split to XHPTransferAttributes or something like that. Keeping like this for now to reduce BC breakage - Allows implementation of non-HTML helpers; until the above modification happens this will involve copypasta, but better than the current situation. - I /think/ this gets rid of the render()/compose() pattern. Fixes facebook/hhvm#130
| if (!($root instanceof HasXHPHelpers)) { | ||
| throw new XHPClassException( | ||
| $this, | ||
| 'compose() must return an :xhp:html-element or :bootstrap:base '. |
| @@ -55,6 +58,7 @@ | |||
| throw new XHPCoreRenderException($this, $that); | |||
| } | |||
|
|
|||
|
|
|||
|
lgtm beyond the whitespace nits that you already fixed. |
|
|
||
| // We want to append classes to the root node, instead of replace them, | ||
| // so do this attribute manually and then remove it. | ||
| if (array_key_exists('class', $attributes) && $attributes['class']) { |
There was a problem hiding this comment.
There should be something on the class to denote that a set of attributes should be appended instead of replaced. For normal HTML that's just class, but there can be others. For instance, Javelin needs data-sigil. What about putting a function in this trait that is like
protected function getAttributesThatAppendValuesOnTransfer(): ImmSet<string> {
return ImmSet {'class'};
}
And you can override it in individual classes or create your own trait that extends this to add your special-needs attributes.
In fact, this trait shouldn't even have it, and there should be an XHPHtmlHelpers trait that extends this and adds the class attribute to the set.
There was a problem hiding this comment.
I'll split this out to an overridable function.
I think changing when this trait would be too big of a change to do so soon. Agree on that naming being better, but I'd rather keep it as is until a 3.0 seems on the horizon.
- add test for class attribute behavior
|
If this looks good, I'll rebase+squash before merge. |
| $thisValue = array_key_exists($attr, $attributes) | ||
| ? (string) $attributes[$attr] | ||
| : ''; | ||
| if ($rootValue !== '' && $thisValue !== '') { |
There was a problem hiding this comment.
I think this needs to be || since otherwise the root's class could be overwritten by an empty string, right?
A better solution might be to make it null otherwise and check for that instead.
There was a problem hiding this comment.
Good catch - this fails:
public function testClassesNotOverridenByEmptyString(): void {
$x = <test:with-class-on-root class="" />;
$this->assertSame('<div class="rootClass"></div>', $x->toString());
}
|
Fixed, and seems fairly backwards-compatible - xhp-bootstrap still works fine (though it's render() is redundant) |
| } | ||
| $this->removeAttribute($attr); | ||
| } | ||
| } |
There was a problem hiding this comment.
How about just
foreach ($this->getAttributesThatAppendValuesOnTransfer() as $attr) {
if (array_key_exists($attr, $attributes)) {
$rootValue = (string)$root->getAttribute($attr);
if ($rootValue !== '') {
$root->setAttribute($attr, $rootValue.' '.(string)$attributes[$attr]);
$this->removeAttribute($attr);
}
}
}
A little less processing for something that could potentially be called many hundreds of times per request.
|
Merging this in tomorrow if there's no further comments. |
|
👍🏻 |
without transferAttributes
the bulk of the transfer/copyattribtues functiontionality split to
XHPTransferAttributes or something like that. Keeping like this for now to
reduce BC breakage
this will involve copypasta, but better than the current situation.
Fixes #130