IBX-4767: Input_text updated extrabutton #682
Conversation
Gengar-i
changed the title
src/bundle/Resources/views/themes/admin/ui/component/input_text.html.twig
Outdated
Show resolved
Hide resolved
|
|
||
| extraBtns.forEach((btn) => { | ||
| const input = btn.closest('.ibexa-input-text-wrapper').querySelector('input'); | ||
| const clearButton = btn.previousElementSibling; |
There was a problem hiding this comment.
Use of previousElementSibling is error-prone: it is enough to insert some other element in between, and previousElementSibling no longer will be a clear button. Remembering about this will be hard thus it should be changed to something more precise.
| const clearButton = btn.previousElementSibling; | ||
| const clearButtonStyles = global.getComputedStyle(clearButton); | ||
| const clearButtonMarginRight = parseInt(clearButtonStyles.getPropertyValue('margin-right'), 10); | ||
| const clearButtonWidth = parseInt(clearButtonStyles.getPropertyValue('width'), 10); |
There was a problem hiding this comment.
width might not be an integer, but float.
There was a problem hiding this comment.
Than padding also will be float I think :D
| const clearButtonMarginRight = parseInt(clearButtonStyles.getPropertyValue('margin-right'), 10); | ||
| const clearButtonWidth = parseInt(clearButtonStyles.getPropertyValue('width'), 10); | ||
|
|
||
| if (!input && clearButton.classList.contains('ibexa-input-text-wrapper__action-btn--clear')) { |
There was a problem hiding this comment.
Just question: why we are checking this condition? When input can be null? Under what condition clear button will not have ibexa-input-text-wrapper__action-btn--clear class?
There was a problem hiding this comment.
as u said in comments above previousElementSibling now I am sure it will be this element I want
There was a problem hiding this comment.
But you are assigning previousElementSibling to a variable which says it is the clear button. So from that perspective, it does not make sense to check whether the clear button has ibexa-input-text-wrapper__action-btn--clear.
Why we are checking whether input is falsy?
There was a problem hiding this comment.
Since input is not required I am checking if its exist
I fixed the clearButton issue
|
|
||
| const paddingRight = `${btn.offsetWidth + clearButtonMarginRight + clearButtonWidth}px`; | ||
|
|
||
| btn.disabled = true; |
There was a problem hiding this comment.
I'm not sure whether disabled should be handled globally.
There was a problem hiding this comment.
| btn.disabled = true; | |
| btn.disabled = !input.value; |
Maybe this way?
There was a problem hiding this comment.
What if someone wants to add the extra button which is not disabled when input is empty?
Gengar-i
changed the title
Gengar-i
force-pushed
the
ibx-4767-product-collection-block
branch
from
84acaf5 to
0a16ade
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Added input_text updated with extrabutton
Checklist:
mainfor features, the oldest supported for bugs).