Share component code cleanup and refactor #848
Conversation
mikemai2awesome
changed the title
|
⚡ PR built on Travis and deployed a now preview here:
|
⚡ Sauce Labs Test for chrome Passed!
Test Details
|
⚡ Sauce Labs Test for internet explorer Passed!
Test Details
|
⚡ Sauce Labs Test for chrome Passed!
Test Details
|
⚡ Sauce Labs Test for internet explorer Passed!
Test Details
|
|
⚡ PR built on Travis and deployed a now preview here:
|
⚡ Sauce Labs Test for internet explorer Passed!
Test Details
|
⚡ Sauce Labs Test for chrome Passed!
Test Details
|
⚡ Sauce Labs Test for chrome Passed!
Test Details
|
⚡ Sauce Labs Test for internet explorer Passed!
Test Details
|
There was a problem hiding this comment.
A few overall thoughts
-
I know we said we want to change from camelCase to kebab-case for variable names. I'm wary of handling it the way it's done in this PR though-- technically, this is a breaking change, and while it's relatively minor here (the share component is only used in one place that we know of), I think it sets a bad precedent. I would argue we need:
- backwards compatibility (e.g. at the top of the file, a section that coverts old variable names to new so nothing breaks)
- a way of announcing the deprecation and actually making sure it gets updated on the consuming end
- a plan for removing the deprecated cruft in the next major release.
-
FYI, it's harder to manage a PR when it includes work from another PR that hasn't been merged yet (e.g.
bolt-listin this case). There's no perfect solution and it's better to keep working rather then getting blocked, but when possible avoid it (maybe nag the reviewers on the first PR :) )
| {# Share component's custom element wrapper. #} | ||
| <bolt-share | ||
| {% if display %} display="{{ display }}" {% endif %} | ||
| {{ parentAttributes }} |
There was a problem hiding this comment.
parentAttributes are not defined (and the attributes object isn't used).
I still don't fully understand the decision to split attributes into parentAttributes and attributes that go inside the component. Potentially worth a parking lot discussion today unless @mikemai2awesome or @sghoweri has an answer that can be posted here?
There was a problem hiding this comment.
Things like hidden/visuallyhidden, margin, and padding overrides need to be on the outer most container for most scenarios to work as expected.
There was a problem hiding this comment.
Right, I get why you need some attributes on the outer container. But do you necessarily need any of them on the inner container?
| ], | ||
| target: source_target | ||
| } | ||
| })]) %} |
There was a problem hiding this comment.
It's a long story (see #846), but due to autoescaping to prevent XSS vulnerabilities on the Drupal end, you can't use this syntax. Basically, you can't set a variable to the results of the include function:
{% items = items|merge[include(....)] %}
Instead, you have to print the results of include
{% set item %}
// add any includes here with either of these:
{{ include("something.twig") }}
{% include something.twig %}
{% endset %}
{% items = items|merge[item] %}
It's currently a gotcha that Salem and I are trying to figure out how to address. For now, if you just don't use the include() function in component templates (it's okay to use in Pattern Lab), you should be fine.
I will fix this case and push an update.
| {% if copyToClipboard %} | ||
| {% set item %} | ||
| {% include "@bolt-components-copy-to-clipboard/copy-to-clipboard.twig" with copyToClipboard only %} | ||
| {% if copy_to_clipboard %} |
There was a problem hiding this comment.
@mikemai2awesome why don't you just pass in the copy_to_clipboard data directly to that component?
That should eliminate the need to map anything (and would also mean the copy_to_clipboard component could get updates that wouldn't require going back in and updating the share component).
Have you tried something like this?
{% if copy_to_clipboard %}
{% set copy_to_clipboard_include %}
<div class="c-bolt-share__copy">
{% include "@bolt-components-copy-to-clipboard/copy-to-clipboard.twig" with copy_to_clipboard only %}
</div>
{% endset %}
{% set items = items|merge([item]) %}
{% set items = items|merge([copy_to_clipboard_include]) %}
{% endif %}That should allow you to then do something like this when you're trying to use it (note the "text" prop is nested inside of the copy_to_clipboard data):
{% include "@bolt-components-share/share.twig" with {
sources: [
{
name: "facebook",
url: "https://www.facebook.com/sharer/sharer.php?u=https://5abd3062df99537b79e29496--bolt-design-system.netlify.com/pattern-lab/patterns/02-components-action-blocks-05-action-blocks/02-components-action-blocks-05-action-blocks.html&src=sdkpreparse"
},
{
name: "twitter",
url: "https://twitter.com/intent/tweet?url=https://bolt-design-system.com&text=Sample%20Share%20Text&via=pega&hashtags=boltDesignSystemRocks!"
},
{
name: "linkedin",
url: "https://www.linkedin.com/shareArticle?url=https://5abd3062df99537b79e29496--bolt-design-system.netlify.com/pattern-lab/patterns/02-components-action-blocks-05-action-blocks/02-components-action-blocks-05-action-blocks.html"
},
{
name: "email",
url: "mailto:?&body=Sample%20Text%20--%20https%3A//bolt-design-system.com"
}
],
copy_to_clipboard: {
url: "http://pega.com",
text: "Copy share link"
}
} only %}There was a problem hiding this comment.
But I want to define the text to be Copy share link though, and have it translatable. If it comes from the schema default, it's not translated. Also, in this case, I am wrapping something around the text so I can style some hover animation.
This feels dirty, I need a better way:
{% include "@bolt-components-copy-to-clipboard/copy-to-clipboard.twig" with {
text: "<span class='c-bolt-share__link-text'>#{copy_to_clipboard_text}</span>",
url: copy_to_clipboard_url,
attributes: {
class: "c-bolt-share__link"
}
} only %}
|
⚡ PR built on Travis and deployed a now preview here:
|
⚡ Sauce Labs Test for chrome Passed!
Test Details
|
⚡ Sauce Labs Test for internet explorer Passed!
Test Details
|
⚡ Sauce Labs Test for chrome Passed!
Test Details
|
⚡ Sauce Labs Test for internet explorer Passed!
Test Details
|
…1-share-component-refactor
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
…bds-311-share-component-refactor
remydenton
force-pushed
the
feature/bds-311-share-component-refactor
branch
from
15a7cf2 to
79330d2
Compare
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
|
⚡ PR built on Travis and deployed a now preview here:
|
|
When you deprecate something in a PR, it's important to follow all the steps in part I of the deprecation wiki page Missing pieces here:
not:
required:
- copyToClipboard
Yes, deprecation is a pain in the butt. |
|
⚡ PR built on Travis and deployed a now preview here:
|
Jira
http://vjira2:8080/browse/BDS-311
Summary
Refactored share component's code to be more efficient.
inlineandcopyToClipboardhave been deprecated.sizeandopacityhave been added as props to allow more flexibility.Details
copyToClipboardprop has been renamed tocopy_to_clipboard.inlineprop has been removed because button version is not being utilized.sizeprop has been added to control text and icon size, and spacing.opacityprop has been added to control the initial transparency of the share tool.View demo
How to test
Pull down the branch and run locally. Navigate to pattern lab and view the share component. Test all the links and copy to clipboard function.
Notes