-
Notifications
You must be signed in to change notification settings - Fork 14
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
Inline Modal Playbook Additions #1467
Conversation
…s, add ids and refs to Flex kit
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.
Checked it out a bit in the review env and everything looks good in all browsers. Let's get this one to alpha testing next!
<span> | ||
{text} | ||
<If condition={removeIcon}> | ||
<span |
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.
Is there a way to make this a button?
@@ -166,6 +166,9 @@ const datePickerHelper = (config) => { | |||
picker.input.style.caretColor = 'transparent' | |||
picker.input.style.cursor = 'pointer' | |||
} | |||
|
|||
// Fix event bubbling bug on wrapper |
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.
Stupid I know but could you change this to "fixes"? I read it like a "Fix this" note.
const css = classnames( | ||
`pb_form_pill_kit_${'primary'}`, | ||
globalProps(props), | ||
className | ||
className, | ||
size === 'small' ? 'small' : null, |
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.
I think you could rewrite this using the classnames functionality. { 'small': size === 'small' }
const FormPillSize = (props) => { | ||
return ( | ||
|
||
<div> |
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.
Can this be a fragment?
@@ -1,7 +1,7 @@ | |||
<%= content_tag(:div, | |||
id: object.id, | |||
data: object.data, | |||
class: object.classname) do %> | |||
class: object.classname + object.size_class) do %> |
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.
I could be totally wrong but I think you could get away with this in the class generator. See below...
@@ -14,6 +15,10 @@ def classname | |||
def display_text | |||
name.downcase | |||
end | |||
|
|||
def size_class | |||
size == "small" ? " small" : "" |
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.
size == "small" ? "small" : null
@@ -6,6 +6,7 @@ class FormPill < Playbook::KitBase | |||
prop :avatar_url | |||
prop :name | |||
prop :text | |||
prop :size | |||
|
|||
def classname | |||
generate_classname("pb_form_pill_kit", "primary", name, text) |
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.
generate_classname("pb_form_pill_kit", "primary", name, text, size_class)
@@ -53,6 +55,7 @@ const TextInput = ( | |||
const dataProps = buildDataProps(data) | |||
const css = classnames([ | |||
'pb_text_input_kit', | |||
inline ? 'inline' : null, |
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.
small classnames function I mentioned above.
@@ -41,6 +43,10 @@ def validation_data | |||
def error_class | |||
error ? " error" : "" | |||
end | |||
|
|||
def inline_class | |||
inline ? " inline" : "" |
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.
inline ? "inline" : null
You don't need the space I think if you use null instead of an empty string.
closeProps={removeProps} | ||
marginRight="xs" | ||
name={label} | ||
size={multiKit === 'smallPill' ? 'small' : ''} |
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.
I think size={multiKit === 'smallPill' && 'small'}
I think this will work for this. I could be wrong.
@@ -22,6 +31,10 @@ def classname | |||
generate_classname("pb_typeahead_kit") | |||
end | |||
|
|||
def inline_class |
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.
Include, if possible, in the generator like I mentioned.
What does this PR do?
Breaking Changes
Only adding props/features but needs regression testing.
Runway Ticket URL
Runway Story
How to test this
Check doc examples for each affected kit (listed above). Currently being alpha tested in Nitro.
Checklist:
enhancement
,bug
,improvement
,new kit
,deprecated
, orbreaking
. See Changelog & Labels for details.Milano
label when you are ready for a review.The normal release cut off deadline is 3p EDT each week. Please reach out to the release team if you have an urgent request or need a release off cycle.