On 04/23/2010 07:57 AM, Denis Gervalle wrote: > On Thu, Apr 22, 2010 at 21:44, Sergiu Dumitriu<[email protected]> wrote: > >> On 04/22/2010 09:29 AM, Denis Gervalle wrote: >>> On Wed, Apr 21, 2010 at 12:35, Jerome Velociter<[email protected]> >> wrote: >>> >>>> Hello Denis, >>>> >>>>> Hi devs, >>>>> >>>>> We have a customer request for highlighting the filters that are >>>> currently >>>>> applied on a livetable. The rationale of this, is that sometimes the >>>>> filtered table is empty or unexpected by the user, but he does not >> notice >>>>> that a filter is currently applied. We have written a proposed >>>>> implementation as well and you may also see what it looks like in the >>>>> attached screenshot, for both text field and select box (for webkit). >>>>> >>>>> Currently, I choose yto use the "background highlight color" of the >>>> active >>>>> color theme, but my feeling is that adding another highlight color >> would >>>>> be >>>>> better. >>>> >>>> I agree >>> >>> >>> Well, since I have not followed what have been discuss regarding the >> color >>> theme, what would be the process to add a new color in it ? How do we >> expect >>> to manage the evolution of the color theme to avoid it to became >> cluttered ? >>> Should we open a discussion on that if none have been already ? >> >> Color themes should be kept small, so that it's easy to design a new >> color theme, and it keeps things in harmony. And, another aspect is that >> different elements of the wiki should use similar colors, so that the UI >> is familiar. We have a highlight color so that users can remember it and >> recognize a highlight no matter where it appears. And I really don't >> think that a skin that uses all the colors of the rainbow would look >> nice and would really be useful. Thus, when designing the components of >> the color theme, we tried to use as few as possible, while still >> allowing creativity. When a color is not enough, remember that you can >> still create variations of it by playing with the opacity (this is what >> we did for the poll to simulate disabled colors). >> > > I completely agree with your analysis. The problem here is that highlight is > also used for sorting column, and IMO this is confusing or not so much > highlighted when the same column is filtered and sorted. But this is matter > of taste anyway. If most you say the highlight color is enough, than I will > use it. > > >> >>> >>>>> >>>>> I have also hook to events separately from the "main" refresh handler, >>>>> because I do not understand the implementation of this handler. Why >> using >>>>> an >>>>> intermediate makeRefreshHandler, and why it does not properly provide >> the >>>>> currently changed filter to the handler ? Could or should I change that >> ? >>>> >>>> Yes you can. The refresh handler should probably not return a function >> but >>>> be a livetable function that is bound as event listener, i.e : >>>> >>>> this.handleRefresh.bindAsEventListener(this) >>>> >>>> versus the current >>>> >>>> this.makeRefreshHandler(this) >>>> >>>> This way you will be able to access the event from the handler, thus the >>>> filter that triggered it. >>>> >>> >>> Ok, I will adapt my implementation and the current code. >>> >>> >>>> One last thing : you should not need the try {} catch {} statements in >>>> your code. I don't see where it could fail (and if it could, I think >> it's >>>> better to try to fix it so that it does not happen). Try catch blocks >> IMO >>>> should be used only in very specific occasions (for example when you >> know >>>> code can fail under certain circumstances, but you don't want to stop >> the >>>> execution). Anyway that's just my opinion, we've never stated on this as >> a >>>> best practice, but we can discuss it in another thread if needed. I'd be >>>> curious to get input from Sergiu about this for example. >>>> >>> >>> I agree. My habits was that minor JS function (those that do not impact >>> usability) should never ever crash, so I usually protect them with >> try/catch >>> in all cases. This is maybe overkill here. Should we also open a >> discusion >>> on this best practice ? Sergiu ? >> >> I agree that JS code should genrally not use try catch. I prefer to >> check the data (if condition holds true, do this) than to assume the >> data is there and is valid, and watch out for exceptions. One advantage >> of pre-data checks is that partial code can't fail midway, while a >> try-catch must cleanup in the end. >> > > Agreed also, this is not the intend here, my intend was just to avoid any > "unexpected" issue from a minor feature. > I will remove them since there no reason for it fails. > > Note that the keyup handler is not the best option IMO. It would fail if >> the user copy-pastes text using the mouse. Additionally, the keyup is >> executed too often, maybe onchange would be enough, although this means >> that the highlight won't be applied until the use moves the focus out of >> the field. >> > > Following Jerome remarks, I have cleanup the refresh handler used for filter > and use for all, so that highlighting is clearly linked with the filtering > its self. Currently, filtering use the "keyup" event for input type=text to > trigger the refresh of the table, this has issues you mention, you are > invited to file a JIRA for that :), this is not the goal of my proposal to > do so. > > >> Maybe a better idea would be to: >> - apply highlight onfocus, even if the input is empty, and keep it that >> way as long as the input still has focus >> - onblur decide if the highlight needs to be kept or not > > > Why not, but why bother while on the same event we do ajax and many heavier > stuffs... than just updating a style ? I also find better to be tightly > linked with the real refresh, so pasting with the mouse currently does not > highlight nor it trigger the refresh and the real filtering. > > If nobody see any more issue, I would like commit the attached patch in 2.4, > should this still deserve a vote ?
No, go ahead. -- Sergiu Dumitriu http://purl.org/net/sergiu/ _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
