Skip to content

feat(input): add start and end slots #28397

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.

Sign up for GitHub

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

Merged
averyjohnston merged 13 commits into from
Nov 27, 2023
Merged

feat(input): add start and end slots #28397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d2bb3e4
update slotMutationController to allow multiple slot names
Oct 20, 2023
1792644
add slots and tests
Oct 20, 2023
47163b5
always float label if slot content is present
Oct 20, 2023
affd46e
make clicking label focus input even if there's a button in the start…
Oct 23, 2023
ac2be10
add comments to labelShouldFloat
Oct 23, 2023
ba03215
lint
Oct 23, 2023
02bd324
add slotted buttons to a11y test
Oct 23, 2023
dc814d3
add e2e tests
Oct 23, 2023
2401b70
chore(): add updated snapshots
Ionitron Oct 23, 2023
91721a5
more comments
Oct 26, 2023
ea352dd
revert padding around buttons in test HTML
Oct 26, 2023
4381cb5
add tech debt ticket
Nov 14, 2023
62cd6f0
lint
Nov 14, 2023
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
8 changes: 2 additions & 6 deletions core/src/components/input/input.md.outline.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@
/**
* This makes the label sit above the input.
*/
:host(.has-focus.input-fill-outline.input-label-placement-floating) .label-text-wrapper,
:host(.has-value.input-fill-outline.input-label-placement-floating) .label-text-wrapper,
:host(.input-fill-outline.input-label-placement-stacked) .label-text-wrapper {
:host(.label-floating.input-fill-outline) .label-text-wrapper {
@include transform(translateY(-32%), scale(#{$form-control-label-stacked-scale}));
@include margin(0);

Expand Down Expand Up @@ -214,8 +212,6 @@
* the floating/stacked label. We simulate this "cut out"
* by removing the top border from the notch fragment.
*/
:host(.has-focus.input-fill-outline.input-label-placement-floating) .input-outline-notch,
:host(.has-value.input-fill-outline.input-label-placement-floating) .input-outline-notch,
:host(.input-fill-outline.input-label-placement-stacked) .input-outline-notch {
:host(.label-floating.input-fill-outline) .input-outline-notch {
border-top: none;
}
4 changes: 1 addition & 3 deletions core/src/components/input/input.md.solid.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@
// Input Label
// ----------------------------------------------------------------

:host(.input-fill-solid.input-label-placement-stacked) .label-text-wrapper,
:host(.has-focus.input-fill-solid.input-label-placement-floating) .label-text-wrapper,
:host(.has-value.input-fill-solid.input-label-placement-floating) .label-text-wrapper {
:host(.label-floating.input-fill-solid.input-label-placement-floating) .label-text-wrapper {
/**
* Label text should not extend
* beyond the bounds of the input.
Expand Down
18 changes: 15 additions & 3 deletions core/src/components/input/input.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,9 @@

flex-grow: 1;

// ensure start/end slot content is vertically centered
align-items: center;

width: 100%;
}

Expand Down Expand Up @@ -624,9 +627,7 @@
/**
* This makes the label sit above the input.
*/
:host(.input-label-placement-stacked) .label-text-wrapper,
:host(.has-focus.input-label-placement-floating) .label-text-wrapper,
:host(.has-value.input-label-placement-floating) .label-text-wrapper {
:host(.label-floating) .label-text-wrapper {
@include transform(translateY(50%), scale(#{$form-control-label-stacked-scale}));

/**
Expand All @@ -635,3 +636,14 @@
*/
max-width: calc(100% / #{$form-control-label-stacked-scale});
}

// Start/End Slots
// ----------------------------------------------------------------

::slotted([slot="start"]) {
margin-inline-end: $form-control-label-margin;
Copy link
Contributor

@liamdebeasi liamdebeasi Oct 26, 2023

Choose a reason for hiding this comment

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

The focus state looks a bit odd on the button:
image

Maybe we need to add some padding?

Copy link
Contributor

@liamdebeasi liamdebeasi Oct 26, 2023

Choose a reason for hiding this comment

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

Oh actually it looks like the padding was removed in the test template. Any particular reason for this?

Copy link
Contributor Author

@averyjohnston averyjohnston Oct 26, 2023

Choose a reason for hiding this comment

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

I just thought it looked nicer 😆 With the default padding added to the margins, it looks like there's a ton of empty space around the icons. I'll revert the padding so the behavior is more clear.

Copy link
Contributor

@liamdebeasi liamdebeasi Oct 30, 2023

Choose a reason for hiding this comment

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

Oh I see why you added it 😂

image

Yeah that does look strange. We might want to consider adding some internal styles to account for this. cc @brandyscarney might have ideas on how best to achieve this. I'd expect the start button to line up with the stacked label at the very least.

Copy link
Member

@brandyscarney brandyscarney Oct 30, 2023

Choose a reason for hiding this comment

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

I looked into this a bit and is there a reason we are recommending buttons for both slots? In the Material Design web catalog they use plain icons. The latest Material Design web components does include a button in the end slot for their "Password" field, although their padding is nonexistent, but I suspect it just hasn't been properly designed yet.

Maybe we should add some styles for slotted icons to make them appear larger and match? Here is what I see with an ion-icon in the start slot and an ion-button in the end slot:

localhost_3333_src_components_input_test_slot(Pixel 5)

In addition, maybe we could style the .button-has-icon-only like this:

:host(.button-has-icon-only) .button-native {
  @include padding(0);

  aspect-ratio: 1;
  border-radius: 50%;
}

Changing the icon in the start slot back to a button, with the above styles, gives me:

localhost_3333_src_components_input_test_slot(Pixel 5) (3)
localhost_3333_src_components_input_test_slot(Pixel 5) (2)

This still looks a bit off because of the left padding, but if we reduce that it also moves the notch. We could remove the left margin for button and get it a bit closer:

localhost_3333_src_components_input_test_slot(Pixel 5)

I think we should focus more on aligning icons in the start slot with the label though. It doesn't seem to be a typical pattern to have a button slotted at the start. Thoughts?

Copy link
Contributor Author

@averyjohnston averyjohnston Oct 31, 2023

Choose a reason for hiding this comment

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

That's a good point that a button in the start slot isn't going to be a common use case. I didn't make the test page with recommended patterns in mind -- the buttons were mostly to check the focus behavior -- but I agree that the icons should be the priority, at least as far as the start slot goes.

That said, I'm hesitant to add too much built-in styling since I don't want devs to have to deal with potentially unwanted extra "magic," especially since they can add their own styling to the slotted content. For example, if we removed the padding from icon-only buttons by default, that would make non-clear buttons look odd:

non-clear buttons

Maybe we just adjust the test to make it seem less like I'm trying to recommend using clear buttons in both slots?

Copy link
Member

@brandyscarney brandyscarney Nov 1, 2023

Choose a reason for hiding this comment

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

It should still look fine with the CSS I added above:

localhost_3333_src_components_input_test_slot(Pixel 5) (1)

but I understand if we don't want to add custom styles just for this. I do think we should consider it in the future though, because Material Design 3 has a section dedicated to icon buttons that looks like the above: https://m3.material.io/components/icon-buttons/overview

Maybe this is something we could add for our Material Design 3 work.

Their spec doesn't show buttons in an input at all so yeah I am fine with pushing this off: https://m3.material.io/components/text-fields/specs

We can also push off styling icons in an input but I think we should revisit that for MD3 too.

Copy link
Contributor Author

@averyjohnston averyjohnston Nov 27, 2023

Choose a reason for hiding this comment

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

We decided to handle this as part of a separate ticket, starting with this spike: https://ionic-cloud.atlassian.net/browse/FW-5645

}

::slotted([slot="end"]) {
margin-inline-start: $form-control-label-margin;
}
44 changes: 39 additions & 5 deletions core/src/components/input/input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import { getCounterText } from './input.utils';
* @virtualProp {"ios" | "md"} mode - The mode determines which platform styles to use.
*
* @slot label - The label text to associate with the input. Use the `labelPlacement` property to control where the label is placed relative to the input. Use this if you need to render a label with custom HTML. (EXPERIMENTAL)
* @slot start - Content to display at the leading edge of the input. (EXPERIMENTAL)
* @slot end - Content to display at the trailing edge of the input. (EXPERIMENTAL)
*/
@Component({
tag: 'ion-input',
Expand Down Expand Up @@ -363,7 +365,7 @@ export class Input implements ComponentInterface {
const { el } = this;

this.legacyFormController = createLegacyFormController(el);
this.slotMutationController = createSlotMutationController(el, 'label', () => forceUpdate(this));
this.slotMutationController = createSlotMutationController(el, ['label', 'start', 'end'], () => forceUpdate(this));
this.notchController = createNotchController(
el,
() => this.notchSpacerEl,
Expand Down Expand Up @@ -684,18 +686,42 @@ export class Input implements ComponentInterface {
}

private renderInput() {
const { disabled, fill, readonly, shape, inputId, labelPlacement } = this;
const { disabled, fill, readonly, shape, inputId, labelPlacement, el, hasFocus } = this;
const mode = getIonMode(this);
const value = this.getValue();
const inItem = hostContext('ion-item', this.el);
const shouldRenderHighlight = mode === 'md' && fill !== 'outline' && !inItem;

const hasValue = this.hasValue();
const hasStartEndSlots = el.querySelector('[slot="start"], [slot="end"]') !== null;

/**
* If the label is stacked, it should always sit above the input.
* For floating labels, the label should move above the input if
* the input has a value, is focused, or has anything in either
* the start or end slot.
*
* If there is content in the start slot, the label would overlap
* it if not forced to float. This is also applied to the end slot
* because with the default or solid fills, the input is not
* vertically centered in the container, but the label is. This
* causes the slots and label to appear vertically offset from each
* other when the label isn't floating above the input. This doesn't
* apply to the outline fill, but this was not accounted for to keep
* things consistent.
*
* TODO(FW-5592): Remove hasStartEndSlots condition
*/
const labelShouldFloat =
labelPlacement === 'stacked' || (labelPlacement === 'floating' && (hasValue || hasFocus || hasStartEndSlots));

return (
<Host
class={createColorClasses(this.color, {
[mode]: true,
'has-value': this.hasValue(),
'has-focus': this.hasFocus,
'has-value': hasValue,
'has-focus': hasFocus,
'label-floating': labelShouldFloat,
Copy link
Contributor Author

@averyjohnston averyjohnston Oct 23, 2023

Choose a reason for hiding this comment

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

Open to alternative ideas for the name of this class.

Copy link
Member

@brandyscarney brandyscarney Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this name makes sense. We tend to do <component>-<status> when naming them. Like the following:

select-disabled
select-expanded
radio-checked
radio-disabled

If you really wanted to you could add input before it since we have the following in other components (radio, select) when talking about the label placement:

[`radio-label-placement-${labelPlacement}`]: true,

[`input-fill-${fill}`]: fill !== undefined,
[`input-shape-${shape}`]: shape !== undefined,
[`input-label-placement-${labelPlacement}`]: true,
Expand All @@ -704,9 +730,16 @@ export class Input implements ComponentInterface {
'input-disabled': disabled,
})}
>
<label class="input-wrapper">
{/**
* htmlFor is needed so that clicking the label always focuses
* the input. Otherwise, if the start slot has something
* interactable, clicking the label would focus that instead
* since it comes before the input in the DOM.
*/}
<label class="input-wrapper" htmlFor={inputId}>
{this.renderLabelContainer()}
<div class="native-wrapper">
<slot name="start"></slot>
<input
class="native-input"
ref={(input) => (this.nativeInput = input)}
Expand Down Expand Up @@ -761,6 +794,7 @@ export class Input implements ComponentInterface {
<ion-icon aria-hidden="true" icon={mode === 'ios' ? closeCircle : closeSharp}></ion-icon>
</button>
)}
<slot name="end"></slot>
</div>
{shouldRenderHighlight && <div class="input-highlight"></div>}
</label>
Expand Down
10 changes: 9 additions & 1 deletion core/src/components/input/test/a11y/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,15 @@ <h1>Input - a11y</h1>
<ion-input label="Email" label-placement="stacked" value="hi@ionic.io"></ion-input><br />
<ion-input label="Email" label-placement="floating"></ion-input> <br />
<ion-input label="Email" label-placement="floating" fill="outline" value="hi@ionic.io"></ion-input> <br />
<ion-input label="Email" label-placement="floating" fill="solid" value="hi@ionic.io"></ion-input>
<ion-input label="Email" label-placement="floating" fill="solid" value="hi@ionic.io"></ion-input><br />
<ion-input label="Email" fill="solid" value="hi@ionic.io">
<ion-button slot="start" aria-label="button">
<ion-icon slot="icon-only" name="lock-closed" aria-hidden="true"></ion-icon>
</ion-button>
<ion-button slot="end" aria-label="button">
<ion-icon slot="icon-only" name="lock-closed" aria-hidden="true"></ion-icon>
</ion-button>
</ion-input>
</main>
</body>
</html>
Loading