-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
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.
The component looks good to me, i just found minor things that can be easily fixed. Thanks!
{% set _id = id|default('') %} | ||
{% set _disabled = disabled|default(false) %} | ||
{% set _invalid = invalid|default(false) %} | ||
{% set _invalid_icon_label = invalid_icon_label|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.
We don't need this in this component, right? (_invalid_icon_label) It's also in the "notes".
- "hide_label" (boolean) (default: '') | ||
- "placeholder" (string) (default: '') | ||
- "rows" (int) (default: 4) | ||
- "extra_classes" (optional) (string) (default: '') Extra classes (space separated) for the icon |
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 wrongly refers to icon, extra_attributes as well
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.
Renders well, I think it's ready to be merged. Questions added for what I noticed going through the code while making the review.
|
||
{% set _css_class = ['ecl-text-area'] %} | ||
{% if _invalid %} | ||
{% set _css_class=_css_class|merge(['ecl-text-area--invalid']) %} |
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.
Not a blocker, yet it "hurts" the eyes not having spaces between assignments. Later in the same document, there are spaces. Too pity there is no prettier for that ...
placeholder="{{- _placeholder -}}" | ||
class="{{- _css_class -}}" | ||
{{- _disabled ? "disabled" -}} | ||
{{- _extra_attributes|raw -}} |
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.
Quickly checked a very-old implementation. It has aria-multiline
and role="textbox"
attributes. Seems like they could be useful for assisting technologies, would it make sense to include them by 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.
Good point... I don't know if they have been remove on purpose in ECL 2.
I'll check
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.
As it seems, the role="textbox"
should not be used in textarea (first paragraph here, and also https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea#Technical_summary)
In a similar way, aria-multiline
seems to be used only to mimic a textarea, but not in actual textareas.
So I'd leave the implementation as it is.
Made a new review, checking previous points.
No description provided.