Convert blockquote to web component #984
Conversation
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
apps/pattern-lab/src/_patterns/02-components/blockquote/35-blockquote-web-components.twig
Outdated
Show resolved
Hide resolved
| // return 'bordered-vertical'; | ||
| // } | ||
| // } | ||
| validateProps(propData) { |
There was a problem hiding this comment.
todo: this feels about ready to promote to either a JS decorator we can tack on before the Class (like @define is, but perhaps with params for this one) or via adding it to the Bolt base component Class
There was a problem hiding this comment.
@sghoweri I moved the validateProps method to the base class in this commit. The way we're calling it in the render function I thought this worked better than a decorator, but perhaps you had more refactoring in mind? Just let me know if you have suggestions. Thanks!
| // If the initial <bolt-link> element contains a link, break apart the original HTML so we can retain the a tag but swap out the inner content with slots. | ||
|
|
||
| // Make sure the button component ONLY ever reuses any existing HTML ONCE. This, in part, helps to prevent rendering diff errors in HyperHTML after booting up! | ||
| if (this._wasInitiallyRendered === false) { |
There was a problem hiding this comment.
todo: this also is starting to feel like a common reoccurring pattern to share across multiple types of components dealing with this. my thought would be to abstract this out to being a util function for now that accepts an array of tag names to check for and returns back the pre-processed and normalized child element.
There was a problem hiding this comment.
| rendered() { | ||
| super.rendered(); // ensure any events emitted by the Bolt Base class fire as expected | ||
|
|
||
| // re-render if Shadow DOM is supported and enabled; temp workaround to dealing w/ components already rendered, but without slot support |
There was a problem hiding this comment.
@danielamorse are we having to do any extra template logic / CSS classes based on any slotted content? If not, is this extra mutation observer actually needed?
There was a problem hiding this comment.
@sghoweri removed, see current version of this file.
| "publishConfig": { | ||
| "access": "public" | ||
| }, | ||
| "dependencies": { | ||
| "ajv": "^6.5.4", |
There was a problem hiding this comment.
@danielamorse same as with the other schema validation comment above -- this dependency should get moved up to Bolt Core as soon as we package up and share schema validation logic at a higher level
There was a problem hiding this comment.
@sghoweri Moved to Bolt Core here, after moving validation method to base class.
…nto feature/convert-blockquote-to-web-component
…teProps method on base class
…nto feature/convert-blockquote-to-web-component
|
⚡ PR built on Travis and deployed a now preview here:
|
…nto feature/convert-blockquote-to-web-component # Conflicts: # packages/core/renderers/bolt-base.js # packages/core/utils/get-component-root-element.js # yarn.lock
…nto feature/convert-blockquote-to-web-component
…ge when JS is loading
…tern Lab-specific pages
… title's Twig to use a cite tag to match JS template
…hen child node mutations are observed
…eb-component-suggestions WIP: Blockquote Refactor Suggestions
…ibutes, copy edits
| foreach ($source as $key => $value) { | ||
| // If $key is not in $target, or if $key is in $target and $value in $source is empty, add/overwrite $key in $target | ||
| // NOTE: empty() and is_null() do not work in the second half of this statement. Why is that? | ||
| if (empty($target[$key]) || (!empty($target[$key]) && $value != "")) { |
There was a problem hiding this comment.
Your code comment says
If $key is not in $target, or if $key is in $target and $value in $source is empty
Your code does this:
If $key is not in $target, or if $key is in $target and $value in $source is NOT empty
Maybe that had something to do with why empty() wasn't working?
In any case, I would replace the whole thing with the following for clarity. Further reading on the array_merge PHP function.
return new Twig_SimpleFunction('merge_attributes', function($target, $source) {
return array_merge($target, $source);
}There was a problem hiding this comment.
And upon further thought, if that's all the custom twig function is doing, we could probably just use the twig merge function instead.
Along those lines, I think it's best to avoid creating custom twig functions that get used within component twig when we can (if they're just used in pattern lab, anything goes) because any custom twig functions also have to work in Drupal. That is, if you pulled the file into Drupal as you have it, it would break with an error about merge_attributes being undefined, and the Drupal team would have to copy this custom function over to their codebase AND maintain it there (i.e. make sure it stays in sync with Bolt's). That problem could potentially be solved by having a system for sharing functions from Bolt to Drupal, but we don't have that AFAIK.
Finally (and sorry to be such a naysayer here), I want to check that merging attribute arrays still results in something that is usable in the same way on the Drupal end. Testing that now.
…tra whitespace causing issues in blockquote
There was a problem hiding this comment.
Very very nice work @danielamorse!
I know we're still iterating on the new pattern for setting up props / prop validation (and we should sneak in a few Jest tests before merging) but this is more than done enough to get my 👍👍
Jira
Summary
Convert
bolt-blockquoteto a web component.Details
Extend
bolt-blockquotecomponent so that blockquotes can be rendered both by twig and via web component. Document any changes.How to test