Display delete action when hovering over read-only term-input entries #249

ncosta-ic · 2025-02-19T10:41:43Z

This PR changes the way by which the user gets hinted about the possibility to remove an entry out of a read-only list of term-input values.

Resolves: #243

sukhwinder33445 · 2025-02-21T09:14:36Z

asset/css/search-base.less

@@ -273,12 +284,19 @@ fieldset:disabled .term-input-area [data-drag-initiator] {
    outline-offset: ~"calc(-@{labelPad} + 3px)";
  }

+  &:not(.vertical) {
+    --delete-action-height: ~"calc(2em + 1.75px)";


Please add a comment, what is calculated here?

As this rule should only apply when read only, you can add it down below.
And you do't need to create a var. you can apply as follows:

&.read-only { ... &:not(.vertical) .delete-action { height: ~"calc(2em + 1.75px)"; } }

sukhwinder33445 · 2025-02-21T09:17:25Z

asset/css/variables.less

@@ -162,6 +164,8 @@
    --search-term-highlighted-bg: var(--primary-button-bg);
    --search-term-highlighted-color: var(--default-text-color-inverted);
    --search-term-drag-border-color: var(--base-gray);
+    --search-term-delete-action-color: var(--base-remove-bg);
+    --search-term-delete-action-color-bg: var(--default-text-color-inverted);


Please name the variable same as the less variable (--search-term-delete-action-bg).

sukhwinder33445 · 2025-02-21T09:44:51Z

asset/css/variables.less

@@ -66,6 +66,8 @@
 @search-term-highlighted-bg: @base-primary-bg;
 @search-term-highlighted-color: @default-text-color-inverted;
 @search-term-drag-border-color: @base-gray;
+@search-term-delete-action-bg: @state-critical;


Please introduce @default-remove-bg and @default-delete-bg and initialize these to @state-critical.
Then use the @default-remove-bg instead of @state-critical here.

As we have the Remove button for now, please change @search-term-**delete**-action-bg to remove.

Please do the same for css variables.

I don't really see the benefit here, as long as there's no multiple references to it. Same could be said about line 61 and 62 🤔

asset/css/variables.less

asset/js/widget/TermInput.js

sukhwinder33445 · 2025-02-21T10:12:30Z

src/FormElement/TermInput/TermContainer.php

                );
+                $label->addHtml(new HtmlElement('span', Attributes::create(['class' => 'invalid-reason'])));


You can add it in the above addHtml call, after .delete-action element.

This would add it as a children of remove-action, which I don't want.

No, the addHtml accepts multiple parameters, so you can add it like this:

$label->addHtml( ... .delete-button, new HtmlElement('span', Attributes::create(['class' => 'invalid-reason'])) );

But that's optional, it's fine the way it is.

sukhwinder33445 · 2025-02-21T10:51:11Z

src/FormElement/TermInput.php

@@ -384,7 +384,8 @@ protected function termContainer(): TermContainer
    {
        if ($this->termContainer === null) {
            $this->termContainer = (new TermContainer($this))
-                ->setAttribute('id', $this->getName() . '-terms');
+                ->setAttribute('id', $this->getName() . '-terms')
+                ->setAttribute('delete-action-label', $this->translate('Remove'));


This looks like an internal thing for TermContainer and it is only required when the input is read-only.

Please add it in the TermContainer::assemble() as follows:

... if ($this->input->getReadOnly()) { $removeLabel = $this->translate('Remove'); $this->setAttribute('delete-action-label', $removeLabel); .... new Icon('trash'), new HtmlElement( 'span', Attributes::create(['class' => 'delete-action-label']), new Text($removeLabel) ) ... }

asset/css/search-base.less

asset/js/widget/TermInput.js

asset/css/search-base.less

sukhwinder33445 · 2025-03-03T16:33:09Z

asset/css/variables.less

@@ -36,11 +36,17 @@
 @base-primary-bg: #00C3ED;
 @base-primary-dark: #0081a6;
 @base-primary-light: fade(@base-primary-bg, 50%);
+@base-remove-bg: @state-critical;
+@base-delete-bg: @base-remove-bg;


We do not need @base-*-bg, @default-*-bg is sufficient, as these are only used to initialise @default- vars.
Please remove these variables.

sukhwinder33445 · 2025-03-03T16:44:55Z

asset/css/search-base.less

+  background: var(--search-term-remove-action-bg, @search-term-remove-action-bg);
+  color: var(--search-term-remove-action-color, @search-term-remove-action-color);
+  border: none;
+  border-radius: 0.25em;


We have .rounded-corners() mixin for radius.

sukhwinder33445 · 2025-03-03T16:57:16Z

src/FormElement/TermInput/TermContainer.php

@@ -33,6 +38,11 @@ public function __construct(TermInput $input)

    protected function assemble()
    {
+        if ($this->input->getReadOnly()) {


You already have the same if condition below, please add it there.

I'm not going to set that attribute for each loop iteration...

Ohh, somehow I overlooked the loop scope. You're right.
Optional: I would still omit the call getAttribute()->getValue() for the label.

sukhwinder33445 · 2025-03-03T17:03:04Z

src/FormElement/TermInput/TermContainer.php

+                            new HtmlElement(
+                                'span',
+                                Attributes::create(['class' => 'remove-action-label']),
+                                new Text($this->getAttribute('remove-action-label')->getValue())


This becomes superfluous once you have merged the if condition above. You can save the translated version in a variable and use it for attribute value and as $content for new Text(). This is what I have already suggested here.

sukhwinder33445 · 2025-03-03T17:05:56Z

src/FormElement/TermInput/TermContainer.php

+                            Attributes::create([
+                                'class' => 'delete-action-content'
+                            ]),
+                            ...Html::wantHtmlList([


I think you have overlooked this.

sukhwinder33445 · 2025-03-03T17:53:16Z

asset/css/search-base.less

+  align-items: center;
+  background: var(--search-term-remove-action-bg, @search-term-remove-action-bg);
+  color: var(--search-term-remove-action-color, @search-term-remove-action-color);
+  border: none;


Not required, as you have never defined a border, so you do not need to remove it.

sukhwinder33445 · 2025-03-03T18:11:28Z

asset/css/search-base.less

@@ -264,6 +276,10 @@ fieldset:disabled .term-input-area [data-drag-initiator] {
      }
    }
  }
+
+  &.read-only [data-index] > .remove-action {


Please do not set a higher specificity here. It should be the same as the rule for non-vertical. So please remove >.

sukhwinder33445 · 2025-03-03T18:15:05Z

asset/css/search-base.less

@@ -277,8 +293,11 @@ fieldset:disabled .term-input-area [data-drag-initiator] {
    [data-index] {
      position: relative;

+      &:not(:hover) > .remove-action {


Please move it a few lines down, after or before the definition of .remove-action for better readability.

sukhwinder33445 · 2025-03-03T18:24:42Z

asset/css/search-base.less

@@ -75,6 +75,18 @@ fieldset:disabled .term-input-area [data-drag-initiator] {
  }
 }

+.remove-action {
+  display: flex;
+  text-align: center;


text-align seems to be superfluous.

sukhwinder33445 · 2025-03-03T18:28:34Z

asset/css/search-base.less

+        visibility: visible;
+        position: absolute;
+        z-index: 1;
+        width: 100%;
+        top: 0;
+        left: 0;


You can move all these properties, which are always required (vertical or not), to the above definition for better readability.

Or even better, the above definition can be merged into this one, as the ‘Remove’ button is only needed if the input field is read-only.

What definitions are you referring to with "above definition"? The one starting on line 78? I placed it there because it's considered to be style relevant and not layout relevant.

ncosta-ic self-assigned this Feb 19, 2025

cla-bot bot added the cla/signed label Feb 19, 2025

ncosta-ic marked this pull request as draft February 19, 2025 10:41

ncosta-ic force-pushed the feature/term-input-delete-action-redesign branch from eba0e21 to 86e2e36 Compare February 19, 2025 14:29

ncosta-ic requested a review from sukhwinder33445 February 19, 2025 14:29

ncosta-ic marked this pull request as ready for review February 19, 2025 14:30

sukhwinder33445 requested changes Feb 21, 2025

View reviewed changes

ncosta-ic added 5 commits March 3, 2025 17:08

feat: delete action hint over read-only term-input entries

5849d5a

fix: only bind remove action label in readOnly mode

a27d125

refactor: isolate removal colors into own variables

3e4c011

refactor, fix: rename delete to 'remove', fix pointer cursor, reorder

823f6d9

fix: missing padding, height calculation correction

282600b

ncosta-ic force-pushed the feature/term-input-delete-action-redesign branch from 86e2e36 to 282600b Compare March 3, 2025 16:10

ncosta-ic requested a review from sukhwinder33445 March 3, 2025 16:10

sukhwinder33445 requested changes Mar 3, 2025

View reviewed changes

ncosta-ic added 3 commits March 4, 2025 10:45

fix: remove superflous css rules, rearrange code

69d6006

feat: remove unneeded base variables

2dfb600

refactor: split removeLabel into own variable

aacdb66

ncosta-ic requested a review from sukhwinder33445 March 4, 2025 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display delete action when hovering over read-only term-input entries #249

Display delete action when hovering over read-only term-input entries #249

ncosta-ic commented Feb 19, 2025

sukhwinder33445 Feb 21, 2025 •

edited

Loading

sukhwinder33445 Feb 21, 2025

sukhwinder33445 Feb 21, 2025

ncosta-ic Mar 3, 2025

sukhwinder33445 Feb 21, 2025

ncosta-ic Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Feb 21, 2025 •

edited

Loading

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

ncosta-ic Mar 4, 2025

sukhwinder33445 Mar 4, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 3, 2025

sukhwinder33445 Mar 4, 2025

ncosta-ic Mar 4, 2025 •

edited

Loading

		);
		$label->addHtml(new HtmlElement('span', Attributes::create(['class' => 'invalid-reason'])));

Display delete action when hovering over read-only term-input entries #249

Are you sure you want to change the base?

Display delete action when hovering over read-only term-input entries #249

Conversation

ncosta-ic commented Feb 19, 2025

sukhwinder33445 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukhwinder33445 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncosta-ic Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

sukhwinder33445 Feb 21, 2025 •

edited

Loading

sukhwinder33445 Feb 21, 2025 •

edited

Loading

ncosta-ic Mar 4, 2025 •

edited

Loading