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

js/Add search term attributes #70

Closed

Conversation

sukhwinder33445
Copy link
Contributor

No description provided.

Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Besides my comments:

  • If the term direction is vertical
    • I think pressing Delete or Backspace should have no effect while in the main input.
    • If Backspace is pressed on a term it should also be ignored, but Delete should probably still remove the term.
    • Pushing Ctrl+a, copying and pasting content should be ignored (makes no sense)
  • If the terms are read only
    • The events onKeyUp, onInputBlur, onTermFocusOut and onTermFocus are completely useless and shouldn't run
  • Override the methods isReadOnlyMode and isTermDirectionVertical in class FilterInput and return always false there. It isn't adjusted to support these and it makes no sense at this time anway.

asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
@sukhwinder33445 sukhwinder33445 force-pushed the add-search-term-attributes branch 2 times, most recently from fe42110 to 09b78fb Compare May 6, 2022 10:35
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Took only a look at the JS changes yet.

asset/js/widget/FilterInput.js Outdated Show resolved Hide resolved
asset/js/widget/TermInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
asset/js/widget/BaseInput.js Outdated Show resolved Hide resolved
@sukhwinder33445 sukhwinder33445 force-pushed the add-search-term-attributes branch 2 times, most recently from 3071739 to 08511c0 Compare May 16, 2022 08:27
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Also:

  • The trash icon should only appear upon hover

asset/js/widget/TermInput.js Outdated Show resolved Hide resolved
asset/js/widget/TermInput.js Outdated Show resolved Hide resolved
asset/js/widget/TermInput.js Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
src/Control/SimpleSuggestions.php Outdated Show resolved Hide resolved
@sukhwinder33445 sukhwinder33445 force-pushed the add-search-term-attributes branch 3 times, most recently from d17b807 to 5352d14 Compare August 29, 2022 13:59
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

  • Pasting content auto submits, however you have disabled it, seems to be ignored
    • Also exact matches and pushing enter submits the form. This may be the default behavior, but the TermInput should treat this as "autosubmit" and then act depending on whether that's disabled or not.
  • Stylewise, the search field should look the same as the search bar. The terms as well. The search icon should also be the same, though not necessarily have the same color. (It's not interactive after all)
    • The trash icon should apply a blur effect. The input should have a slightly darker backgorund once hovered.

src/Control/SimpleSearchField.php Show resolved Hide resolved
src/Control/SimpleSuggestions.php Outdated Show resolved Hide resolved
src/Control/SimpleSuggestions.php Outdated Show resolved Hide resolved
src/Control/SimpleSearchField.php Outdated Show resolved Hide resolved
@sukhwinder33445 sukhwinder33445 force-pushed the add-search-term-attributes branch 8 times, most recently from 01f074a to 09b7953 Compare September 6, 2022 12:26
@sukhwinder33445 sukhwinder33445 force-pushed the add-search-term-attributes branch 4 times, most recently from d13b349 to 569cafd Compare October 13, 2022 11:46
@nilmerg nilmerg force-pushed the add-search-term-attributes branch 2 times, most recently from 9961b93 to 1f90cc1 Compare April 4, 2023 10:06
@nilmerg nilmerg changed the base branch from main to enhance-term-input April 4, 2023 10:07
@nilmerg nilmerg force-pushed the enhance-term-input branch 3 times, most recently from a9ee49e to 5816848 Compare April 21, 2023 10:40
@nilmerg nilmerg force-pushed the enhance-term-input branch 3 times, most recently from 803eadf to d0f4a75 Compare April 28, 2023 08:08
Base automatically changed from enhance-term-input to main April 28, 2023 08:20
@nilmerg nilmerg force-pushed the add-search-term-attributes branch from 0f326b5 to 23a8ed9 Compare May 3, 2024 14:45
@nilmerg nilmerg changed the base branch from main to read-only-term-input May 3, 2024 14:46
@nilmerg nilmerg force-pushed the add-search-term-attributes branch from 23a8ed9 to 530d463 Compare May 3, 2024 16:40
@nilmerg nilmerg force-pushed the add-search-term-attributes branch from 530d463 to d2c7b49 Compare May 6, 2024 13:02
@nilmerg
Copy link
Member

nilmerg commented May 6, 2024

I believe this is obsolete now. A generic input is available since #139, the vertical direction is possible since #179 and the read only mechanism will be with #222.

@nilmerg nilmerg closed this May 6, 2024
@nilmerg nilmerg deleted the add-search-term-attributes branch May 6, 2024 13:06
@nilmerg nilmerg removed their request for review May 6, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants