Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075453002:
WebUI cr-search-field: Trigger event after programmatically clearing query. (Closed)
|
Created:
4 years, 6 months ago by dpapad Modified:
4 years, 6 months ago CC:
chromium-reviews, dbeam+watch-elements_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI cr-search-field: Trigger event after programmatically clearing query.
This fixes a regression in MD History where the results were not updated
after the user clicks the 'x' button.
BUG=620250
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0a48717579ccbf96d3adc62f0c831d226761ddf1
Cr-Commit-Position: refs/heads/master@{#400072}
Patch Set 1 #Patch Set 2 : Add test. #
Messages
Total messages: 17 (6 generated)
Description was changed from ========== MD History: Trigger results update after clearing search field. BUG=620250 ========== to ========== MD History: Trigger results update after clearing search field. BUG=620250 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD History: Trigger results update after clearing search field. BUG=620250 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== WebUI cr-search-field: Trigger event after programmatically clearing query. This fixes a regression in MD History where the results were not updated after the user clicks the 'x' button. BUG=620250 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + tsergeant@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
ok, let's take a step back
why can't all of this be accomplished with polymer properties / observers?
properties: {
value: {
observer: 'valueChanged_',
type: String,
value: '',
},
},
valueChanged_: function() {
this.fire('search-changed', this.value);
},
any code that does if (newValue == this.oldValue) or onValueUpdated_() is
duplicating logic with polymer core, essentially...
On 2016/06/15 at 22:49:18, dbeam wrote:
> ok, let's take a step back
>
> why can't all of this be accomplished with polymer properties / observers?
>
> properties: {
> value: {
> observer: 'valueChanged_',
> type: String,
> value: '',
> },
> },
>
> valueChanged_: function() {
> this.fire('search-changed', this.value);
> },
>
> any code that does if (newValue == this.oldValue) or onValueUpdated_() is
duplicating logic with polymer core, essentially...
The underlying text field value is already stored inside the iron-input element
'value' and 'bindValue' properties. <iron-input> element communicates changes
via a 'search'
event, which is not triggering when the value is changed programmatically, and
also it not smart enough to not fire the 'search' event if the value did not
change.
Your suggestion creates a redundant member variable holding state inside this
behavior, that belongs to the <iron-input> element. Current approach only tracks
lastValue_, which is not redundant, since as said above is not tracked anywhere.
On 2016/06/15 23:00:32, dpapad wrote:
> On 2016/06/15 at 22:49:18, dbeam wrote:
> > ok, let's take a step back
> >
> > why can't all of this be accomplished with polymer properties / observers?
> >
> > properties: {
> > value: {
> > observer: 'valueChanged_',
> > type: String,
> > value: '',
> > },
> > },
> >
> > valueChanged_: function() {
> > this.fire('search-changed', this.value);
> > },
> >
> > any code that does if (newValue == this.oldValue) or onValueUpdated_() is
> duplicating logic with polymer core, essentially...
>
> The underlying text field value is already stored inside the iron-input
element
> 'value' and 'bindValue' properties. <iron-input> element communicates changes
> via a 'search'
> event, which is not triggering when the value is changed programmatically, and
> also it not smart enough to not fire the 'search' event if the value did not
> change.
>
> Your suggestion creates a redundant member variable holding state inside this
> behavior, that belongs to the <iron-input> element. Current approach only
tracks
> lastValue_, which is not redundant, since as said above is not tracked
anywhere.
are we essentially looking for the combined behavior of "change" (i.e. kind of
de-duping) and "search" (i.e. rate limited)?
this probably means we have to do one part of this logic ourselves (either some
setTimeout()s or ignoring Enter or duplicate values).
I vote de-duping, but I think adding a "searchTerm_" property and just setting
it on each search (and observing the property and fire()ing) is using the
platform more polymerically.
On 2016/06/15 at 23:26:20, dbeam wrote:
> On 2016/06/15 23:00:32, dpapad wrote:
> > On 2016/06/15 at 22:49:18, dbeam wrote:
> > > ok, let's take a step back
> > >
> > > why can't all of this be accomplished with polymer properties / observers?
> > >
> > > properties: {
> > > value: {
> > > observer: 'valueChanged_',
> > > type: String,
> > > value: '',
> > > },
> > > },
> > >
> > > valueChanged_: function() {
> > > this.fire('search-changed', this.value);
> > > },
> > >
> > > any code that does if (newValue == this.oldValue) or onValueUpdated_() is
> > duplicating logic with polymer core, essentially...
> >
> > The underlying text field value is already stored inside the iron-input
element
> > 'value' and 'bindValue' properties. <iron-input> element communicates
changes
> > via a 'search'
> > event, which is not triggering when the value is changed programmatically,
and
> > also it not smart enough to not fire the 'search' event if the value did not
> > change.
> >
> > Your suggestion creates a redundant member variable holding state inside
this
> > behavior, that belongs to the <iron-input> element. Current approach only
tracks
> > lastValue_, which is not redundant, since as said above is not tracked
anywhere.
>
> are we essentially looking for the combined behavior of "change" (i.e. kind of
de-duping) and "search" (i.e. rate limited)?
There are two parts to the equation.
The 1st one is de-duping yes. We do this by keeping track of the lastValue_
ourselves (introduced by https://codereview.chromium.org/2057353002).
>
> this probably means we have to do one part of this logic ourselves (either
some setTimeout()s or ignoring Enter or duplicate values).
There is no need for direct usage of setTimeout(), iron-input already takes care
of deciding when to fire a "search" event if the user stops typing for a certain
period of time.
>
> I vote de-duping, but I think adding a "searchTerm_" property and just setting
it on each search (and observing the property and fire()ing) is using the
platform more polymerically.
The 2nd part of the equation is the origin of the change. A change can happen in
two ways.
1) User types in the search field ('search' event is fired).
2) The search field is modified programmatically (clicking the 'x'), no 'search'
event is fired by the underlying HTML element, but we take this into account and
fire our own event 'search-changed' ourselves (if necessary).
The searchTerm_ approach requires us to update searchTerm_ manually every time
the value is changed either via 1 or 2. I don't see this being clearly better
than just firing manually an event when the value was
changed via 2 only.
Either way, can we defer any refactorings here for later?
This is simply fixing a regression following the same overall approach that
cr_search_field_behavior was already using and is adding a test to cover for it.
lgtm with our in-person talk (sorry that this didn't make sense to me at first)
On 2016/06/16 at 00:07:07, dbeam wrote: > lgtm with our in-person talk (sorry that this didn't make sense to me at first) Thanks!
lgtm
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075453002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== WebUI cr-search-field: Trigger event after programmatically clearing query. This fixes a regression in MD History where the results were not updated after the user clicks the 'x' button. BUG=620250 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== WebUI cr-search-field: Trigger event after programmatically clearing query. This fixes a regression in MD History where the results were not updated after the user clicks the 'x' button. BUG=620250 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0a48717579ccbf96d3adc62f0c831d226761ddf1 Cr-Commit-Position: refs/heads/master@{#400072} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0a48717579ccbf96d3adc62f0c831d226761ddf1 Cr-Commit-Position: refs/heads/master@{#400072} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
