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.

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

Jump to bottom

feat(text-editor): implement label prop #2929

Merged
merged 2 commits into from
Apr 25, 2024
Merged

feat(text-editor): implement label prop #2929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6941841
feat(text-editor): implement `label` prop
Kiarokh Apr 24, 2024
6c60475
fix(text-editor): ensure clicking anywhere on the text editor activat…
Kiarokh Apr 25, 2024
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
14 changes: 14 additions & 0 deletions src/components/text-editor/examples/text-editor-composite.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,13 @@ export class TextEditorCompositeExample {
@State()
private readonly = false;

@State()
private label: string;

public render() {
return [
<limel-text-editor
label={this.label}
value={this.value}
onChange={this.handleChange}
readonly={this.readonly}
Expand All @@ -26,6 +30,11 @@ export class TextEditorCompositeExample {
label="Readonly"
onChange={this.setReadonly}
/>
<limel-input-field
label="Label"
value={this.label}
onChange={this.handleLabelChange}
/>
</limel-example-controls>,
<limel-example-value value={this.value} />,
];
Expand All @@ -36,6 +45,11 @@ export class TextEditorCompositeExample {
this.readonly = event.detail;
};

private handleLabelChange = (event: CustomEvent<string>) => {
event.stopPropagation();
this.label = event.detail;
};

private handleChange = (event: CustomEvent<string>) => {
this.value = event.detail;
};
Expand Down
7 changes: 6 additions & 1 deletion src/components/text-editor/prosemirror-adapter/partial-styles/_prosemirror-menu.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@use '../../../../style/mixins';
@use '../../../style/internal/shared_input-select-picker.scss';

$size-of-caret: 0.25rem;

Expand All @@ -7,6 +8,11 @@ div#editor {
position: sticky !important;
z-index: 1;
top: 0;
margin-left: -0.75rem;
margin-right: -0.75rem;
background-color: shared_input-select-picker.$background-color-normal;
backdrop-filter: blur(0.25rem);
-webkit-backdrop-filter: blur(0.25rem);

&[style*='position: fixed'] {
// They add this dynamically as soon as you scroll.
Expand Down Expand Up @@ -34,7 +40,6 @@ div#editor {
align-items: center;

padding: 0.125rem;
background-color: rgb(var(--contrast-100));
border-top-left-radius: 0.5rem;
border-top-right-radius: 0.5rem;
}
Expand Down
6 changes: 1 addition & 5 deletions src/components/text-editor/prosemirror-adapter/partial-styles/_prosemirror.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@
font-variant-ligatures: none;
font-feature-settings: 'liga' 0;

border-bottom-left-radius: 0.5rem;
border-bottom-right-radius: 0.5rem;

padding: var(--limel-text-editor-padding);
background-color: rgb(var(--contrast-100));
padding-top: 0.75rem;

[draggable][contenteditable='false'] {
user-select: text;
Expand Down
16 changes: 1 addition & 15 deletions src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
@use '../../../style/internal/shared_input-select-picker.scss';

:host(limel-text-editor) {
:host(limel-prosemirror-adapter) {
isolation: isolate;
display: block;
}
Expand All @@ -10,20 +8,8 @@
}

.ProseMirror-menubar-wrapper {
transition: border 0.2s ease;
display: grid;
grid-template-rows: auto 1fr;
border-radius: 0.25rem;
border: 1px solid;
border-color: shared_input-select-picker.$lime-text-field-outline-color;

&:hover {
border-color: shared_input-select-picker.$lime-text-field-outline-color--hovered;
}

&:focus-within {
border-color: shared_input-select-picker.$lime-text-field-outline-color--focused;
}
}

.ProseMirror-textblock-dropdown {
Expand Down
16 changes: 12 additions & 4 deletions src/components/text-editor/text-editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
:host(limel-text-editor) {
--limel-text-editor-padding: 0.5rem 1rem;
display: flex;
flex-direction: column;
width: 100%;
}

limel-markdown {
display: block;
padding: var(--limel-text-editor-padding);
fieldset {
min-width: 0;
min-height: 0;

:host(limel-text-editor[readonly]) & {
padding-block-start: 0.75rem;
}
}

@import '../../style/internal/fieldset.scss';
15 changes: 14 additions & 1 deletion src/components/text-editor/text-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,12 @@ export class TextEditor implements FormComponent<string> {
public change: EventEmitter<string>;

public render() {
return this.renderEditor();
return (
<fieldset disabled={this.readonly || this.disabled}>
{this.renderLabel()}
{this.renderEditor()}
</fieldset>
);
}

private renderEditor() {
Expand All @@ -97,6 +102,14 @@ export class TextEditor implements FormComponent<string> {
);
}

private renderLabel() {
if (!this.label) {
return;
}

return <legend>{this.label}</legend>;
}

private handleChange = () => (event: CustomEvent<string>) => {
event.stopPropagation();
this.change.emit(event.detail);
Expand Down
62 changes: 62 additions & 0 deletions src/style/internal/fieldset.scss
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adrianschmidt this might be interesting for you

Why is this a new file, and in located in style/internal?

Because with some minor CSS magic, you can turn the <fieldset> element to something that looks like our the famous MDC input field.

The only difference is that we don't get that transition (floating animation) of the label → floating_label.

But also, we don't have to make that complex structure of mdc-notched-outline
image
and handle all the focus/floating related stuff.

My long term plan for us is to get rid of the MDC logic in limel-input-field. I think using field-set is a good way out. Which is why I am putting this in a separate file, which we can hopefully reuse later.

Semantics and accessibility?

Note

Note that to get a better user interface, I need to use the legend to display the label.

As far as I can see in online examples, you can have a single input element in a fieldset.

However, the fieldset and legend elements are typically used to group together several related form controls and label them with a common theme. So when it comes to semantics and accessibility, we may wonder:

  1. whether using a fieldset element, but only putting one single (not several) input fields in it is OK.
  2. Also, whether using a label element for that input field, and instead use the legend that belongs intact to the fieldset and displaying the label of the field in the legend is OK?

Would these make confusing experiences for users of assistive technologies such as screen readers?

Using a fieldset for a single input field is not semantically incorrect, but it might be overkill and could potentially confuse users of assistive technologies.

The legend element is originally meant to provide a caption for the fieldset. Using it as a replacement for a label may not recommended.

My own assessment for the current usage (in limel-text-editor) is that we are just fine. Firstly, the ProseMirror library has lots of accessibility issues, so this one is really minor, to be honest. Secondly, I feel wrapping complexities and controls of a rich text editor inside a fieldset can still make sense (?)

But when/if the day comes that we wanna do the same, when refactoring limel-input-field and use fieldset in there, we should keep in mind that screen readers and other assistive technologies rely on the correct use of HTML elements for proper functionality. Therefore if we still want to use legend instead of label, it might not be correctly associated with the input field by assistive technologies, leading to a confusing user experience.

To mitigate these issues, we could consider the following:

Use a label for the input field, even if we're also using a legend. This ensures that the input field has a proper label that can be read by assistive technologies.

If we want to use the legend to display the label for stylistic reasons, we could hide the label visually but still keep it in the HTML. This way, it can be read by screen readers but won't affect our layout.

Copy link
Collaborator

@adrianschmidt adrianschmidt Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds great to me! 👍

To mitigate these issues, we could consider the following:

Use a label for the input field, even if we're also using a legend. This ensures that the input field has a proper label that can be read by assistive technologies.

If we want to use the legend to display the label for stylistic reasons, we could hide the label visually but still keep it in the HTML. This way, it can be read by screen readers but won't affect our layout.

Reading your post, I was think exactly this. We can definitely have a visually hidden label that is still read by screen readers. We can probably also prevent screen readers from reading out the content of the legend element.

EDIT: We don't need to make those tweaks now. I meant for later, if and when we make the proposed changes to limel-input-field 👍

Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
@use './shared_input-select-picker.scss';
@use '../mixins.scss';

$_thickness-of-the-border: 1px;

fieldset {
box-sizing: border-box;
transition:
border-color 0.2s ease,
background-color 0.2s ease;
border: $_thickness-of-the-border solid;
border-radius: 0.25rem;

margin-inline-start: 0;
margin-inline-end: 0;
padding-block-start: 0;
padding-inline-start: 0.75rem;
padding-inline-end: 0.75rem;
padding-block-end: 0.75rem;

&:not([disabled]) {
border-color: shared_input-select-picker.$lime-text-field-outline-color;
background-color: shared_input-select-picker.$background-color-normal;

&:hover {
border-color: shared_input-select-picker.$lime-text-field-outline-color--hovered;
background-color: shared_input-select-picker.$background-color-hovered;
}

&:focus-within {
border-color: shared_input-select-picker.$lime-text-field-outline-color--focused;
}
}

&[disabled] {
border-color: transparent;
}

&:has(legend) {
// In input fields, the `label`s are optional,
// and we use the `legend` to render the `label`.
// This ensures that when or if the label appears,
// the field doesn't visually move down in the DOM.
$_height-of-the-legend: -0.75rem;
margin-top: calc(
(#{$_height-of-the-legend} / 2) + (#{$_thickness-of-the-border} / 2)
);
}
}

legend {
box-sizing: border-box;
@include mixins.truncate-text;
max-width: 100%;

color: shared_input-select-picker.$label-color;
font-size: 0.65rem; // `10.4px` similar to MDC's floating label
letter-spacing: var(--mdc-typography-subtitle1-letter-spacing, 0.009375em);

padding-inline-start: 0.25rem;
padding-inline-end: 0.25rem;
}
Loading