Add search component #176
Add search component #176
Conversation
| "lib/*" | ||
| ], | ||
| "dependencies": { | ||
| "@tradeshift/elements": "^0.4.5" |
There was a problem hiding this comment.
Please add @tradeshift/elements.icon as dependency
There was a problem hiding this comment.
Good catch! I added it.
| } | ||
|
|
||
| .search-icon { | ||
| margin: 7px; |
There was a problem hiding this comment.
What's the reason for this number?
There was a problem hiding this comment.
Yeah, it actually a result of some non-trivial calculations. I replaced it with calc expression and added a comment with some details.
|
|
||
| .close-icon { | ||
| cursor: pointer; | ||
| margin: 11px; |
There was a problem hiding this comment.
What about this one?
There was a problem hiding this comment.
It is a result of some non-trivial calculations. I replaced it with calc expression and added a comment with some details.
|
|
||
| constructor() { | ||
| super(); | ||
| this.autofocus = false; |
There was a problem hiding this comment.
Is it necessary to define default value for them? Since we have the reflect: true it makes the DOM messy if you set default values!
There was a problem hiding this comment.
I just followed the docs about lit-element static properties getter:
If you implement a static properties getter, initialize your property values in the element constructor.
I remove default values for boolean properties, because the element still working as expected.
|
SonarQube analysis reported 1 issue
Watch the comments in this conversation to review them. |
aleks-elkin
force-pushed
the
elkin/add-search-component
branch
2 times, most recently
from
07c4e37 to
aea6314
Compare
| | dir | dir | string | '' | Direction 'rtl' or 'ltr' | | ||
| | focused | focused | boolean | false | Set the focus on element | | ||
| | idleTime | idle-time | number | 300 | timeout in ms for the 'idle' event | | ||
| | placeholder | placeholder | string | 'Search...' | Message when an input is empty | |
There was a problem hiding this comment.
We should prioritize translation integration for next sprint!!! @Tradeshift/ui
There was a problem hiding this comment.
| } | ||
|
|
||
| handleKeyDown(e) { | ||
| if (e.key === 'Enter') { |
There was a problem hiding this comment.
Not sure if it's causing an issue, @zdlm where we can find the list of the supported browsers by Tradeshift?
https://caniuse.com/#feat=keyboardevent-key
aleks-elkin
force-pushed
the
elkin/add-search-component
branch
from
7c122b0 to
7eea205
Compare
| @focus="${this.handleFocus}" | ||
| @blur="${this.handleBlur}" | ||
| /> | ||
| <div class="icon-container"> |
There was a problem hiding this comment.
Don't have strong opinion about this one but what do you say if we put this part to a getter for cancelIconTemplate or some other name? like this one
There was a problem hiding this comment.
Yes, I think with this separate getter code looks cleaner. I moved the close icon html template to a separate getter.
aleks-elkin
force-pushed
the
elkin/add-search-component
branch
from
3d4629b to
45630c4
Compare
Added a Search component according to #110 issue and provided design.
Main features:
Also, added a generic debounce function to helpers. It increases the bundle's size on 200 bytes.
Screenshots:
Basic search:
Search with some search text:
RTL support: