-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UI Framework][K7]: Form components #13435
Conversation
|
||
return ( | ||
<div> | ||
{options.map(function (option, index) { |
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 have no idea how this should really be handled, right now I use a simple array, but that doesn't allow for things like passing defaultChecked
on the actual input. Figured @cjcenizal could help here. I can say that checkboxes should always be in multiple sets, because switches are the single use checkbox variant.
className={classes} | ||
{...rest} | ||
> | ||
{options.map(function (option, index) { |
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.
Again, dunno the best way to deal with this. Figure @cjcenizal can help here.
ui_framework/dist/ui_framework.css
Outdated
@@ -557,6 +557,284 @@ table { | |||
.kuiCallOutHeader__title { | |||
color: #0079a5; } | |||
|
|||
.kuiForm__textField { |
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 ended up using a base class to style all the similar text fields (text, textarea, password, number...etc). I originally had this as a mixin, but figured having a base class saves us a lot of CSS.
You can see this class passed as a default class on top of every text based input... ex: <input type="password" className="kuiForm__textField kuiFormPassword" />
.
I think that's the right way to do it.
@@ -0,0 +1,83 @@ | |||
// The following code is inspired by... |
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 modified this quite a bit. It's small enough I didn't want to install it as a dependency, because the overwrites would have been just as large as the original source. Let me know if this is the proper way to credit stuff in our code.
Name suggestions:
|
@@ -0,0 +1,69 @@ | |||
import React, { |
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.
We'll only need to pass id
to <KuiFormRow>
(and not to the child component), if we do this:
import React, {
cloneElement,
PropTypes,
} from 'react';
import classNames from 'classnames';
import { KuiIcon } from '../../../components';
export const KuiFormRow = ({ children, icon, helpText, invalid, errors, label, id, className, ...rest }) => {
const classes = classNames(
'kuiFormRow',
className,
{
'kuiFormRow--withIcon' : icon,
'kuiFormRow--invalid' : invalid,
}
);
let optionalIcon = null;
if (icon) {
optionalIcon = <KuiIcon className="kuiFormRow__icon" type={icon} size="medium" />;
}
let optionalHelpText = null;
if (helpText) {
optionalHelpText = <div className="kuiFormRow__helpText">{helpText}</div>;
}
let optionalErrors = null;
if (errors) {
optionalErrors = (
errors.map(function (error, index) {
return <div key={index} className="kuiFormRow__error">{error}</div>;
})
);
}
let optionalLabel = null;
if (label) {
optionalLabel = <label className="kuiFormRow__label" htmlFor={id}>{label}</label>;
}
let field;
if (id) {
field = cloneElement(children, {
id,
});
} else {
field = children;
}
return (
<div
className={classes}
{...rest}
>
{/*
Order is important here. The label needs to be UNDER the children.
We rearrange the flex order in the CSS so the label ends up
displaying above the children / input. This allows us to still
use sibling selectors against the label that are tiggered by the
focus state of the input.
*/}
{field}
{optionalLabel}
{optionalErrors}
{optionalHelpText}
{optionalIcon}
</div>
);
};
KuiFormRow.propTypes = {
label: PropTypes.string,
id: PropTypes.string,
icon: PropTypes.string,
invalid: PropTypes.bool,
errors: PropTypes.array,
helpText: PropTypes.string,
};
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.
This worked and I applied it. The only negative is it won't register when the child has isRequired
on the prop. I went ahead and removed is required from the child components.
OK @cjcenizal I think this is in a pretty good state. I ran tests but a couple of them fail because of required props. Might need a primer on that tomorrow. |
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.
This is beautiful! Great work @snide. Had a few comments but nothing major.
// to be more easily maintained / themable going forward. | ||
|
||
.kuiRange { | ||
-webkit-appearance: none; |
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.
How about just appearance: none
here, and let PostCSS autoprefix it?
// Don't use this, make proper ids instead. This is just for the example. | ||
function makeId() { | ||
return Math.random().toString(36).substr(2, 5); | ||
} |
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.
Note to self: port @BigFunger's ID generator over to the UI Framework.
id={makeId()} | ||
label="Text field in a form row" | ||
> | ||
<KuiFieldText name="text_name_in_row" /> |
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.
Note to self: update examples with ARIA attributes.
}]} | ||
> | ||
<GuideText> | ||
<p> |
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 don't think we need a <p>
here btw.
<button> | ||
Logstash | ||
</button> | ||
Logstash |
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.
Good catch!
$kuiRangeTrackHeight: 2px !default; | ||
$kuiRangeTrackBorderWidth: 0 !default; | ||
$kuiRangeTrackBorderColor: $kuiColorLightShade !default; | ||
$kuiRangeTrackRadius: $kuiBorderRadius !default; |
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.
Just wondering, why are we marking these overrideable?
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.
no need. copy/pasta. ill remove
// MIT License | ||
|
||
// It has been modified to fit the styling patterns of Kibana and | ||
// to be more easily maintained / themable going forward. |
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.
This credit looks good to me, but I think "themable" is spelled "themeable", based on a quick Google search.
@include kuiFieldStyle; | ||
} | ||
|
||
.kuiSelect::-ms-expand { |
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 nested with &
inside of .kuiSelect
?
|
||
|
||
|
||
|
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 any way to set Vim to ensure there's only one empty line at the end of the file? https://www.google.com/search?q=vim+eof+newline&rlz=1C5CHFA_enUS695US695&oq=vim+eof+&aqs=chrome.3.69i57j0l5.5218j0j7&sourceid=chrome&ie=UTF-8
/** | ||
* 1. The label is our main clickable area. It sits above the actual switch. | ||
*/ | ||
.kuiSwitch__label { |
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 we unnest everything from this line down?
@cjcenizal Got everything but the nesting. Can do the tyography stuff in a different PR. Removed radio. I'll need to add that next week. |
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.
LGTM!
Adds form components and example documentation to KUI.
Adds form components and example documentation to KUI.
Adds form components and example documentation to KUI.
Adds form components and example documentation to KUI.
Adds form components.
Todo before merge...
Todo after merge...
Stuff that I need help from @cjcenizal...
KuiForm
wrapper component.Light version
Dark version