Skip to content
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

feat(text-field): Add ripple to outlined text field #1807

Merged
merged 19 commits into from
Dec 21, 2017
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/mdc-textfield/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ Method Signature | Description

##### `MDCTextField.ripple`

The `MDCRipple` instance for the root element that `MDCTextField` initializes when given an `mdc-text-field--box` root element. Otherwise, the field is set to `null`.
`MDCRipple` instance. When given an `mdc-text-field--box` root element, this is set to the `MDCRipple` instance on the root element. When given an `mdc-text-field--outlined` root element, this is set to the `MDCRipple` instance on the `mdc-text-field__outline` element. Otherwise, the field is set to `null`.

### `MDCTextFieldAdapter`

Expand Down
1 change: 1 addition & 0 deletions packages/mdc-textfield/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const cssClasses = {
FOCUSED: 'mdc-text-field--focused',
INVALID: 'mdc-text-field--invalid',
BOX: 'mdc-text-field--box',
OUTLINED: 'mdc-text-field--outlined',
};

export {cssClasses, strings};
28 changes: 17 additions & 11 deletions packages/mdc-textfield/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,6 @@ class MDCTextField extends MDCComponent {
if (labelElement) {
this.label_ = labelFactory(labelElement);
}
this.ripple = null;
if (this.root_.classList.contains(cssClasses.BOX)) {
const MATCHES = getMatchesProperty(HTMLElement.prototype);
const adapter = Object.assign(MDCRipple.createAdapter(this), {
isSurfaceActive: () => this.input_[MATCHES](':active'),
registerInteractionHandler: (type, handler) => this.input_.addEventListener(type, handler),
deregisterInteractionHandler: (type, handler) => this.input_.removeEventListener(type, handler),
});
const foundation = new MDCRippleFoundation(adapter);
this.ripple = rippleFactory(this.root_, foundation);
}
const bottomLineElement = this.root_.querySelector(strings.BOTTOM_LINE_SELECTOR);
if (bottomLineElement) {
this.bottomLine_ = bottomLineFactory(bottomLineElement);
Expand All @@ -120,6 +109,23 @@ class MDCTextField extends MDCComponent {
if (iconElement) {
this.icon_ = iconFactory(iconElement);
}

this.ripple = null;
if (this.root_.classList.contains(cssClasses.BOX) || this.root_.classList.contains(cssClasses.OUTLINED)) {
const MATCHES = getMatchesProperty(HTMLElement.prototype);
const rippleInputMethods = {
isSurfaceActive: () => this.input_[MATCHES](':active'),
registerInteractionHandler: (type, handler) => this.input_.addEventListener(type, handler),
deregisterInteractionHandler: (type, handler) => this.input_.removeEventListener(type, handler),
};
if (this.root_.classList.contains(cssClasses.BOX)) {
const adapter = Object.assign(MDCRipple.createAdapter(this), rippleInputMethods);
const foundation = new MDCRippleFoundation(adapter);
this.ripple = rippleFactory(this.root_, foundation);
} else if (this.root_.classList.contains(cssClasses.OUTLINED)) {
this.ripple = this.outline_.createRipple(rippleFactory, rippleInputMethods);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit overboard to me to have a separate method on the outline subcomponent that does exactly the same thing as above, and requiring us to pass everything through to it, when the only difference is which element the ripple is instantiated on.

I also find it a bit strange since the ripple is still destroyed from the top level component, and there's nothing really "owning" it from the outline subcomponent.

Given that the branching logic is here anyway, should we simply conditionalize which element the ripple is created on here and otherwise have the same code for both in one place?

Another thought: should the decision of which element hosts the ripple be determined within an API for the top-level foundation, so that component implementors have less logic to worry about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the separate method on the outline subcomponent is because we need access to the outline's root element to instantiate the ripple on. The other option was to have a public getRoot() method in the outline, which seems to break best practices....although that might seem less weird than having an entire createRipple() method on a subcomponent that doesn't even own the ripple. Thoughts?

Moving the ripple host decision to the foundation seems even more complicated since the main foundation would have to somehow access the outline subcomponent (don't know if this is possible, maybe through the outline subadapter?...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that we already have access to the outline subcomponent's root element: outlineElement. We just passed it to the subcomponent constructor about 30 lines above in this same method.

And...yeah my idea about making the foundation decide ends up being pretty awkward, so nevermind about that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh, huge brain fart on my part. Updated it to share the same code except for conditionalizing on which element the ripple is created on.

}
}
}

destroy() {
Expand Down
6 changes: 4 additions & 2 deletions packages/mdc-textfield/mdc-text-field.scss
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,10 @@
opacity: 1;
}

&:not(.mdc-text-field--focused) .mdc-text-field__outline-path {
stroke-width: 1px;
&.mdc-text-field--focused .mdc-text-field__outline-path {
@include mdc-theme-prop(stroke, primary);

stroke-width: 2px;
}

&.mdc-text-field--disabled {
Expand Down
62 changes: 52 additions & 10 deletions packages/mdc-textfield/outline/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,64 @@ The outline is a border around all sides of the text field. This is used for the

## Usage

#### MDCTextFieldOutline API
### HTML Structure

##### MDCTextFieldOutline.foundation
```html
<div class="mdc-text-field__outline">
<svg>
<path class="mdc-text-field__outline-path"/>
</svg>
</div>
<div class="mdc-text-field__idle-outline"></div>
```

MDCTextFieldOutlineFoundation. This allows the parent MDCTextField component to access the public methods on the MDCTextFieldOutlineFoundation class.
### Usage within `mdc-text-field`

### Using the foundation class
```html
<div class="mdc-text-field mdc-text-field--outlined">
<input class="mdc-text-field__input" id="my-text-field-id" type="text">
<label class="mdc-text-field__label" for="my-text-field-id">Label</label>
<div class="mdc-text-field__outline">
<svg>
<path class="mdc-text-field__outline-path"/>
</svg>
</div>
<div class="mdc-text-field__idle-outline"></div>
</div>
```

### CSS Classes

CSS Class | Description
--- | ---
`mdc-text-field__outline` | Mandatory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actual descriptions?

`mdc-text-field__outline-path` | Mandatory
`mdc-text-field__idle-outline` | Mandatory

#### `MDCTextFieldOutline`

Method Signature | Description
--- | ---
getWidth() => number | Returns the width of the outline element
getHeight() => number | Returns the height of the outline element
setOutlinePathAttr(value: string) => void | Sets the "d" attribute of the outline element's SVG path
`MDCTextFieldOutline.createRipple(rippleFactory, rippleInputMethods) => MDCRipple` | Returns an `MDCRipple` instance set on the `mdc-text-field__outline` element

##### `MDCTextFieldOutline.foundation`

This allows the parent `MDCTextField` component to access the public methods on the `MDCTextFieldOutlineFoundation` class.

##### `MDCTextFieldOutline.createRipple(rippleFactory: (function(!Element, !MDCRippleFoundation): !MDCRipple), foundation: !MDCRippleFoundation)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we tend to not include the ! in our notation in the READMEs which follows TypeScript syntax.


#### The full foundation API
Returns an `MDCRipple` instance set on the `mdc-text-field__outline` element, which will be used by the parent `MDCTextField` component.

##### MDCTextFieldOutlineFoundation.updateSvgPath(width: number, height: number, labelWidth: number, radius: number, isRtl: boolean)
### `MDCTextFieldOutlineAdapter`

Updates the SVG path of the focus outline element based on the given width and height of the text field element, the width of the label element, the corner radius, and the RTL context.
Method Signature | Description
--- | ---
`getWidth() => number` | Returns the width of the outline element
`getHeight() => number` | Returns the height of the outline element
`setOutlinePathAttr(value: string) => void` | Sets the "d" attribute of the outline element's SVG path

### `MDCTextFieldOutlineFoundation`

Method Signature | Description
--- | ---
`updateSvgPath(labelWidth: number, radius: number, isRtl: boolean) => void` | Updates the SVG path of the focus outline element based on the given the width of the label element, the corner radius, and the RTL context.
12 changes: 12 additions & 0 deletions packages/mdc-textfield/outline/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import MDCComponent from '@material/base/component';
import {MDCRipple, MDCRippleFoundation} from '@material/ripple';

import {strings} from './constants';
import MDCTextFieldOutlineAdapter from './adapter';
Expand All @@ -41,6 +42,17 @@ class MDCTextFieldOutline extends MDCComponent {
return this.foundation_;
}

/**
* @param {(function(!Element, !MDCRippleFoundation): !MDCRipple)=} rippleFactory A function which
* creates a new MDCRipple.
* @param {!Object=} rippleAdapterMethods Adapter method implementations for ripple that should override the default.
*/
createRipple(rippleFactory = (el, foundation) => new MDCRipple(el, foundation), rippleAdapterMethods = {}) {
const adapter = Object.assign(MDCRipple.createAdapter(this), rippleAdapterMethods);
const foundation = new MDCRippleFoundation(adapter);
return rippleFactory(this.root_, foundation);
}

/**
* @return {!MDCTextFieldOutlineFoundation}
*/
Expand Down
13 changes: 9 additions & 4 deletions packages/mdc-textfield/outline/mdc-text-field-outline.scss
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
}

.mdc-text-field__outline {
@include mdc-ripple-surface;
@include mdc-states-base-color(black);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line; the next line will already result in including mdc-states-base-color(text-primary-on-light).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is necessary now that we're using just mdc-states-press-opacity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True (I hadn't tested the behavior prior to writing that comment), but should it be using text-primary-on-light like you were doing with mdc-states, or black?

@include mdc-states(text-primary-on-light);
@include mdc-ripple-radius;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Move this above mdc-states, below mdc-ripple-surface

@include mdc-theme-prop(color, primary);
@include mdc-text-field-outlined-corner-radius($mdc-text-field-border-radius);

Expand All @@ -44,17 +48,18 @@
transition: mdc-text-field-transition(opacity);
opacity: 0;
z-index: 2;
overflow: hidden;

svg {
position: absolute;
width: 100%;
height: 100%;

.mdc-text-field__outline-path {
@include mdc-theme-prop(stroke, primary);

stroke-width: 2px;
transition: mdc-text-field-transition(stroke-width), mdc-text-field-transition(opacity);
stroke: $mdc-text-field-outlined-idle-border;
stroke-width: 1px;
transition: mdc-text-field-transition(stroke), mdc-text-field-transition(stroke-width),
mdc-text-field-transition(opacity);
fill: transparent;
}
}
Expand Down
22 changes: 22 additions & 0 deletions test/unit/mdc-textfield/mdc-text-field-outline.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
*/

import bel from 'bel';
import td from 'testdouble';
import {assert} from 'chai';

import {MDCRipple} from '../../../packages/mdc-ripple';
import {MDCTextFieldOutline} from '../../../packages/mdc-textfield/outline';

const getFixture = () => bel`
Expand All @@ -33,12 +35,32 @@ test('attachTo returns an MDCTextFieldOutline instance', () => {
assert.isOk(MDCTextFieldOutline.attachTo(getFixture()) instanceof MDCTextFieldOutline);
});

class FakeRipple {
constructor(root) {
this.root = root;
this.layout = td.func('.layout');
this.destroy = td.func('.destroy');
}
}

function setupTest() {
const root = getFixture();
const component = new MDCTextFieldOutline(root);
return {root, component};
}

test('#createRipple returns a ripple on the root element', () => {
const {root, component} = setupTest();
const ripple = component.createRipple((el, foundation) => new FakeRipple(root, foundation));
assert.equal(ripple.root, root);
});

test('#createRipple initializes a default ripple when no ripple factory given', () => {
const {component} = setupTest();
const ripple = component.createRipple();
assert.instanceOf(ripple, MDCRipple);
});

test('#adapter.getWidth returns the width of the element', () => {
const {root, component} = setupTest();
const width = component.getDefaultFoundation().adapter_.getWidth();
Expand Down
18 changes: 17 additions & 1 deletion test/unit/mdc-textfield/mdc-text-field.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class FakeLabel {

class FakeOutline {
constructor() {
this.createRipple = td.function('.createRipple');
this.destroy = td.func('.destroy');
}
}
Expand All @@ -87,7 +88,8 @@ test('#constructor when given a `mdc-text-field--box` element instantiates a rip
assert.equal(component.ripple.root, root);
});

test('#constructor sets the ripple property to `null` when given a non `mdc-text-field--box` element', () => {
test('#constructor sets the ripple property to `null` when not given a `mdc-text-field--box` nor ' +
'a `mdc-text-field--outlined` subelement', () => {
const component = new MDCTextField(getFixture());
assert.isNull(component.ripple);
});
Expand All @@ -100,6 +102,20 @@ test('#constructor when given a `mdc-text-field--box` element, initializes a def
assert.instanceOf(component.ripple, MDCRipple);
});

test('#constructor when given a `mdc-text-field--outlined` element, initializes a default ripple when no ' +
'ripple factory given', () => {
const root = bel`
<div class="mdc-text-field mdc-text-field--outlined">
<input type="text" class="mdc-text-field__input" id="my-text-field">
<label class="mdc-text-field__label" for="my-text-field">My Label</label>
<div class="mdc-text-field__outline"></div>
<div class="mdc-text-field__idle-outline"></div>
</div>
`;
const component = new MDCTextField(root);
assert.instanceOf(component.ripple, MDCRipple);
});

test('#constructor instantiates a bottom line on the `.mdc-text-field__bottom-line` element if present', () => {
const root = getFixture();
const component = new MDCTextField(root);
Expand Down