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-editor): implement label prop #2929

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Conversation

Kiarokh
Copy link
Contributor

@Kiarokh Kiarokh commented Apr 24, 2024

Review:

  • Commits are atomic
  • Commits have the correct type for the changes made
  • Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

  • Chrome
  • Edge
  • Firefox

Linux:

  • Chrome
  • Firefox

macOS:

  • Chrome
  • Firefox
  • Safari

Mobile:

  • Chrome on Android
  • iOS

Copy link

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-2929/

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

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 👍

@adrianschmidt
Copy link
Collaborator

I've read Kia's comment and quickly eyed over the PR. Concept-wise it gets the mark of approval from me. I have not tested anything or looked closely at the code changes. I'll leave that to someone from the text editor cycle team 😊

Good work! 🙌

@Kiarokh Kiarokh force-pushed the text-editor-more-props branch 2 times, most recently from 399afb6 to 100de20 Compare April 25, 2024 10:26
@LucyChyzhova LucyChyzhova requested review from LucyChyzhova and removed request for LucyChyzhova April 25, 2024 11:02
@LawrenceBorst LawrenceBorst self-requested a review April 25, 2024 12:55
@LucyChyzhova
Copy link
Contributor

Together with readOnly and an empty composer, it looks strange
Maybe we should improve the readOnly state - like to have opacity: 05 for the whole compose, so we see it somehow? 🤔

Screen.Recording.2024-04-25.at.14.52.46.mov

@Kiarokh
Copy link
Contributor Author

Kiarokh commented Apr 25, 2024

Together with readOnly and an empty composer, it looks strange

Good point. We should do something similar to what we do in the input field.
image

But we can do it later in a follow up PR.

Copy link
Contributor

@LawrenceBorst LawrenceBorst left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm not in a position to delve deep into the css but I see that the css gods have already passed their judgement.

@LucyChyzhova
Copy link
Contributor

We should do something similar to what we do in the input field.

Agree! I made an issue for it https://github.com/Lundalogik/crm-feature/issues/4079

@LucyChyzhova LucyChyzhova merged commit 906707a into main Apr 25, 2024
11 checks passed
@LucyChyzhova LucyChyzhova deleted the text-editor-more-props branch April 25, 2024 13:19
@lime-opensource
Copy link
Collaborator

🎉 This PR is included in version 37.25.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants