-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Share > OG Share Meta Data #552
Conversation
…new component package
…" or "False" instead of "1" and blank
…only works outside an iframe)
…nd validation, updated docs demo
@@ -8,6 +8,27 @@ npm install @bolt/components-share | |||
## Description: | |||
The share component provides the user a visual queue to share the content with relevant peers, as well as an easy mechanism to actively do the sharing. | |||
|
|||
## Share Link Notes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 could you include links to the Facebook, Twitter, & LinkedIn resources for the info below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"@bolt/components-icon": "^1.0.2", | ||
"@bolt/components-link": "^1.0.2", | ||
"@bolt/core": "^1.0.2" | ||
"@bolt/core": "^1.0.0-rc.9", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: bump to latest Bolt version when mergin in
<meta charset="UTF-8"> | ||
<meta name="viewport" content="width=device-width" /> | ||
<title>Bolt Design System</title> | ||
|
||
<!-- Default OG Tag examples --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 can you move this to an include we can pull in?
Basic gut check here is can we include + configure in say, the docs site without having to manually copy and paste this chunk of code in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -1,21 +1,22 @@ | |||
<div class="demo--share"> | |||
<div style="margin: 0 auto; width: 150px; padding: 25rem 0 0;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 can we use utility classes here as a good best practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sghoweri -- I like the idea, but I think we should create a ticket where we go through all demos and swap inline styles for utility classes (since this has been the approach we've used all along while creating demos). Another thought is that no one will really see this markup as these are only intended for demo purposes, but I totally understand wanting to "drink our own cool-aid".
Also -- if we have utility classes for things like margin and padding (I've seen spacing?) -- shouldn't we add these under Styleguide > Utilities in PL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a helluva lot easier to make progress on something by incrementally chipping away at it rather than a giant push to fix everything all at once. ;)
Sure, eating our own dog food is important, however by larger strategy here is actually 4-fold (regardless on if someone is going to see our code for this or not -- these still apply):
-
It lets us continuously validate what sorts of UI problems can get solved via utility classes alone (vs if it's not a Component, sorry!)
-
It allows us to continuously test to make sure our utility classes continue to work as expected
-
It builds up experience across the team so anyone can better provide support and guidance
-
It forces us to evaluate and reevaluate the API / syntax so we can improve over time
@@ -10,6 +10,10 @@ properties: | |||
type: string | |||
description: The text to use as the succesful copy text | |||
default: Copied! | |||
iconSize: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 wouldn't this technically just be using the full set of icons available to the icon component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy to clipboard component very purposefully only uses 2 icons. I wouldn't think we would open this up to allow for any icon to be passed, which is why I didn't pass along all options. Let me know if that doesn't make any sense.
pointer-events: none; | ||
} | ||
.c-bolt-link__icon [name='facebook-solid'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 @theSadowski @mikemai2awesome @joekarasek @EvanLovely as a follow-up todo after we merge this in, we need to chat about how best to handle these sorts of use cases (contextual selectors) -- ie. how do we need to update the Icon component to allow for this sort of thing in a decoupled way.
Add a custom class per icon that gets auto-generated for example?
} | ||
|
||
.c-bolt-block-list__item { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 same with these: need to follow up on how best do we handle this without directly selecting one component inside another
} from '@bolt/core'; | ||
|
||
@define | ||
export class BoltShare extends withHyperHTML() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up todo: update to use updated HyperHTML renderer once #561 is merged in
position: "before", | ||
size: 'medium' | ||
}, | ||
attributes: create_attribute(imageAttributes | default({})).addClass('js-bolt-share__'~source.name).setAttribute('target',sourceTarget) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 can you double check to make sure we're pulling in the correct attributes name here? Shouldn't imageAttributes
instead be attributes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Thank you for catching that!
spacing: "none" | ||
} only %} | ||
content: include ("@bolt-components-block-list/block-list.twig", { | ||
"contentItems" : items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 can we rename the block-list
component to take in items
instead of contentItems
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rockymountainhigh1943 just a couple super tiny things to check out - once those are banged out I think this is good to merge in -- a couple things to follow-up on after all this is done (architecture stuff mostly) -- very nice job!!
⚡ PR built on Travis and deployed a Netlify preview here: https://5ac2ca7292226a2adadcb944--bolt-design-system.netlify.com
|
@sghoweri -- This one is ready for another review! Thinking its ready to merge! :-) |
@rockymountainhigh1943 there's a couple small bugs I'm noticing (unrelated to this PR directly) that we'll need to follow up on, but that shouldn't block getting this merged in and shipped. Nice work!!! |
No description provided.