feat: pfe-button component #837
Conversation
First pass at creating a button.
| return true; | ||
| } | ||
|
|
||
| _parentObserverHandler(mutationList) { |
| background-color: #06c; | ||
| color: #fff; | ||
| font-size: 16px; | ||
| padding: 8px 16px; |
There was a problem hiding this comment.
Feel like this should be em, since it should be proportionate to the font size.
There was a problem hiding this comment.
Prefer we use the pfe-var and pfe-color functions and pull from the theme instead of hardcoding values 👍
There was a problem hiding this comment.
@castastrophe I've updated as many variables as I could, but there were still a few I couldn't find or just didn't know how to use. For example, the red background color for the danger variant is #a30000, which maps to $pf-color-red-200 in_pf-colors.scss, but I couldn't figure out how to pull it into the component 😞. Also, in pfe-apply-broadcast(text), how can I specify that I want the fallback to be #151515 instead of #333333. I thought I could pass it as a second arg, but it screamed at me when I did.
| color: #06c; | ||
|
|
||
| &:after { | ||
| border: 1px solid #06c; |
There was a problem hiding this comment.
CSS var for width and color?
There was a problem hiding this comment.
@wesruv I've added a var for color. Will this work? Or should I specify the width separately too?
$variables: (
...
Border: 0,
Border--after: 1px solid,
BorderColor--after: transparent,
BorderRadius: #{pfe-var(surface--border-radius)},
...
);
|
@kylebuch8 @castastrophe Is further design support and/or documentation needed here? |
|
We are good from the design side, we will be repurposing the same styles from PF. |
|
Is the button height 34px? That's what it looks like here https://deploy-preview-837--happy-galileo-ea79c4.netlify.app/elements/pfe-button/demo/ |
@kylebuch8 Looking at the PF button styles here: https://www.patternfly.org/v4/documentation/react/components/button, it looks like we'll need to import or extend a few form spacing variables in our system since we don't have a 6px spacing variable at the moment. You'll also want to set the line-height for the button to |
Co-authored-by: [ Cassondra ] <[email protected]>
castastrophe
added
demo
…s into pfe-button
Stylesheet suggestions; simplified the interpolation and reduced unnecessary selectors
Moved after to a region
castastrophe
left a comment
castastrophe
left a comment
There was a problem hiding this comment.
Looking great! I went ahead and pushed up 2 simple commits - just cleaning up unneeded interpolation and moving the after identifier to a region rather than a modifier re: the BEM conventions.
| // set the _externalBtn, and initialize the component | ||
| _parentObserverHandler() { | ||
| if (!this._isValidLightDom()) { | ||
| return; |
There was a problem hiding this comment.
@kylebuch8
Just a comment, what happens if it was valid light DOM, but becomes invalid light DOM? Throw an error and maintain current state? Is that the right behavior in that circumstance, was thinking maybe it should get disabled?
wesruv
left a comment
wesruv
left a comment
There was a problem hiding this comment.
I had a comment or two, but looks good to me code-wise!
First pass at creating a button.