@diosmosis opened this Pull Request on July 16th 2017 Member

This PR is based off https://github.com/piwik/piwik/pull/11857

Changes

  • Added generate:angular-component command to generate an empty angular component. Also modified the result of generate:angular-directive to use one way binding.
  • Moved period selector code into a new set of angular directives/components including:

    • piwik-period-selector: encapsulates the entire period selector.
    • piwik-date-picker: extends the jquery UI datepicker component allowing users to highlight/select multiple dates.
    • piwik-period-date-picker: a date picker that highlights/selects all the dates within a configurable period.
    • piwik-date-range-picker: encapsulates two date pickers to pick different ends of a range.

    Changed the strategy for highlighting/selecting whole periods from event based to state based.

  • Created a periods angular service to provide pretty labels & date ranges. Plugins can add to the available period types via piwik.addCustomPeriod().
  • Make sure getAllowedPeriodsFor...() doesn't return duplicate periods.
  • Removed the AJAX loading gif in the period selector (didn't seem to actually be used). old text

Changed behavior

  • After a period is selected, the range start/end date is set to the start/end date for the selected period. This seems to happen before, but not for year periods. W/ this PR the behavior is changed to be consistent, which, at the very least, affects one of the UI tests.
  • The calendar icon shows up now. I don't know why it wasn't showing up before.

Removed behavior

  • There was some behavior to change the period if the label was selected, not the current period and simply clicked. I think it was to get around an issue in the double click handling. I removed it, double clicking works, not sure if single clicking on a selected period should result in loading a new page.
  • Removed some behavior that stopped future reloads if a reload was in progress. I think it's poorer UX & since the period selector closes after a date is selected, there's less of a risk.
@mattab commented on September 11th 2017 Owner

Hi @diosmosis
What is the status of this PR? as you need a Review for it, do you want to update it and resolve conflicts or should we review it already?

@diosmosis commented on September 11th 2017 Member

The code should be ready for review, it's just the screenshot that conflicts.

@diosmosis commented on September 13th 2017 Member

I'll rebase this PR tomorrow.

@tsteur commented on September 24th 2017 Owner

@diosmosis I didn't review the code in detail but noticed this:

  • ui tests show different results
  • ui tests don't seem to show the date picker icon as well?
  • date picker does not close when applying a date

I am seeing those errors in console:
image

Sometimes it seems like history forward and backwards seems to not work for the date selector but this might be related to the JS error.

Also I am thinking it is not updating piwik.currentDateString and piwik.endDateString, piwik. startDateString but not 100% sure it did that before. This could likely cause bugs.

@diosmosis commented on September 26th 2017 Member

@tsteur Interesting, I remember fixing these, maybe I broke something w/ the rebase...

@diosmosis commented on September 28th 2017 Member

@tsteur Fixed two of the issues you mention (not the UI tests not showing the calendar icon issue, waiting for tests for that). Ready for another review.

Also I am thinking it is not updating piwik.currentDateString and piwik.endDateString, piwik. startDateString but not 100% sure it did that before. This could likely cause bugs.

They are updated in the setPiwikPeriodAndDate() function in period-selector.controller.js file.

@diosmosis commented on September 28th 2017 Member

@tsteur looked at the UI tests in this branch and I can't find a test that still doesn't show the icon. What exactly was the issue?

@tsteur commented on September 28th 2017 Owner

@diosmosis the piwik.start/endDateString seems to be not updated when using eg the back button in the browser after changing a date. I think they are also not updated when entering a date in the text field manually (I had select a range period, changed the end date via text field, and then clicked on apply but it did not update start/end date string).

Something that I noticed especially in the exported dashboard (eg https://apache.piwik/index.php?module=Widgetize&action=iframe&moduleToWidgetize=Dashboard&actionToWidgetize=index&idSite=1&period=week&date=2017-09-15 ) but also in the UI itself:
When having period week and selecting a day, it first selects the current day and makes this black and when moving mouse or something it only then highlights the whole selected week with a black background. In current date selector it does this directly.

I'm not sure if this is an existing problem or an older problem (update: I checked and it is not a regression but wondering if possible to fix easily). When requesting eg
https://apache.piwik/index.php?module=CoreHome&action=index&idSite=1&period=range&date=2017-07-07,2017-08-09#?idSite=1&period=day&date=yesterday&category=General_Visitors&subcategory=CustomVariables_CustomVariables

It does correctly fetch the reporting page for CustomVariables for yesterday,day but before that I can see eg a request

https://apache.piwik/index.php?date=2017-07-07,2017-08-09&filter_limit=-1&format=JSON2&idSite=1&method=API.getReportPagesMetadata&module=API&period=range

Where not the correct date is used. I presume the start/end date string or currentDate or something needs to be updated earlier?

Period selector seems to work otherwise. Tested in latest chrome, latest firefox and IE10. Also checked it with exported dashboard and seems to work. Not sure if we use date selector somewhere else.

@diosmosis commented on October 1st 2017 Member

@diosmosis the piwik.start/endDateString seems to be not updated when using eg the back button in the browser after changing a date. I think they are also not updated when entering a date in the text field manually (I had select a range period, changed the end date via text field, and then clicked on apply but it did not update start/end date string).

This should be fixed in the latest commit (moved the responsibility of setting those vars outside of the period selector; now done on location change).

@diosmosis commented on October 1st 2017 Member

Something that I noticed especially in the exported dashboard (eg https://apache.piwik/index.php?module=Widgetize&action=iframe&moduleToWidgetize=Dashboard&actionToWidgetize=index&idSite=1&period=week&date=2017-09-15 ) but also in the UI itself:
When having period week and selecting a day, it first selects the current day and makes this black and when moving mouse or something it only then highlights the whole selected week with a black background. In current date selector it does this directly.

I was able to reproduce this once going from day period to week period, but then it just started working. Seems random, I'll keep looking into it.

@diosmosis commented on October 1st 2017 Member

I'm not sure if this is an existing problem or an older problem (update: I checked and it is not a regression but wondering if possible to fix easily). When requesting eg
https://apache.piwik/index.php?module=CoreHome&action=index&idSite=1&period=range&date=2017-07-07,2017-08-09#?idSite=1&period=day&date=yesterday&category=General_Visitors&subcategory=CustomVariables_CustomVariables

It does correctly fetch the reporting page for CustomVariables for yesterday,day but before that I can see eg a request

https://apache.piwik/index.php?date=2017-07-07,2017-08-09&filter_limit=-1&format=JSON2&idSite=1&method=API.getReportPagesMetadata&module=API&period=range

Where not the correct date is used. I presume the start/end date string or currentDate or something needs to be updated earlier?

Did some digging, looks like this is the issue: https://github.com/piwik/piwik/blob/3.x-dev/plugins/CoreHome/javascripts/calendar.js#L671. I kept that code in my last commit, will remove it since the date parsing is a bit more intelligent now.

@diosmosis commented on October 2nd 2017 Member

Something that I noticed especially in the exported dashboard (eg https://apache.piwik/index.php?module=Widgetize&action=iframe&moduleToWidgetize=Dashboard&actionToWidgetize=index&idSite=1&period=week&date=2017-09-15 ) but also in the UI itself:
When having period week and selecting a day, it first selects the current day and makes this black and when moving mouse or something it only then highlights the whole selected week with a black background. In current date selector it does this directly.

@tsteur Found the issue for this & fixed it, however there's still a flicker when you select a date due to jquery datepicker processing the dates before we process them. Not sure why it didn't happen in the previous version, maybe angular adds more time due to the digest cycle. Will this be an issue?

I tried fixing it by using new CSS classes instead of overriding datepicker CSS styles, so we wouldn't have to wait until the datepicker is done setting CSS classes. But I think the datepicker re-renders the HTML every time it updates.

@tsteur commented on October 2nd 2017 Owner

If we cannot fix it, it will be fine I reckon. Was just a little bit weird looking issue.

@diosmosis commented on October 2nd 2017 Member

I'm going to keep trying to fix it, but I think the real fix will be to upgrade to Angular 2 & use a more flexible date picker. If only there was enough time... :)

@tsteur commented on October 3rd 2017 Owner

@diosmosis check this gif out when range period is selected (latest Chrome). It seems to not highlight the day or not select it.

oct-04-2017 11-23-09

@diosmosis commented on October 5th 2017 Member

@tsteur fixed that bug, two others and changed the rendering strategy of the picker so rendering will be smoother. Only had time to test on chrome today.

The old rendering strategy was to let jquery UI render, then change the CSS classes on the dates. The new strategy is to disable the click event for the date picker cells so they won't trigger a render as often. This way we don't have to wait before rendering (no setTimeouts).

@tsteur commented on October 5th 2017 Owner

@diosmosis I was going to say good to merge but noticed in IE 10 it is not showing any of the selectors (cannot see any error message though):

image

image

@diosmosis commented on October 7th 2017 Member

@tsteur fixed the IE10 issue (used a 'const' by accident). Also found another issue that was fixed in another commit. Tested on chrome, firefox + ie10.

@tsteur commented on October 8th 2017 Owner

Works now in IE10. I noticed one more thing when editing the input directly it does not seem to select the actual date but one day in advance. Not sure if maybe timezone related or so but don't think so. GIF is from IE 10 but might be same in Chrome

oct-09-2017 08-45-46

@diosmosis commented on October 8th 2017 Member

@tsteur I can't reproduce, either on chrome or IE10 (I also tried setting my system timezone to Auckland). If you're willing to help debug on your end, would you be able to see what's going on in the onRangeInputChanged() function in date-range-picker.component.js? I'm assuming the $.datepicker.parseDate call is parsing the date incorrectly, but of course I can't verify.

@tsteur commented on October 8th 2017 Owner

I can reproduce it on IE10 but not Chrome. For IE 10 I'm using browserstack with Win8 and getTimezoneOffset() returns 420 if that helps.

I tried to debug and noticed it only goes in that method when dateStr is incomplete eg 2017-10-0 but not again when the date is fully completed with eg 2017-10-02. Sometimes I can make it go into that method by removing all text and then double clicking or so.

I try to reproduce it by first removing the last digit of a complete date, then typing eg 2.

The date seems to result in [+] date Mon Oct 2 00:00:00 PDT 2017 Object, (Date) if that helps.

@diosmosis commented on October 8th 2017 Member

@tsteur Tried using browserstack win 8 w/ IE 10 (latest) & IE 10 (metro), and they both seem to work. If I remove the last char in a date string, and replace it (w/ '2' or something else), the correct date is selected.

The date seems to result in [+] date Mon Oct 2 00:00:00 PDT 2017 Object, (Date) if that helps.

Sounds like the date's being parsed correctly. Only other thing I can think of to check is in setDateCellColor() in date-picker.js. In this if statement:

https://github.com/diosmosis/piwik/blob/ce4c5dab3cd5f2a29b767bf03d9c0e771df3e5ad/plugins/CoreHome/angularjs/date-picker/date-picker.directive.js#L303

can you put console.log(scope.selectedDateStart, scope.selectedDateEnd, dateValue, $dateCellLink.text()); and see what is printed out when you change the date text?

No rush though, given I can't reproduce this.

@tsteur commented on October 8th 2017 Owner

It is weird, now it seems to longer update the date sometimes at all

oct-09-2017 11-43-55

also check out this behaviour where it sometimes does not accept a date, but then when you make the date invalid it accepts it and sometimes it seems like using cursor back key doesn't work for some reason even though it should:

(cannot upload 20MB please wait)

@tsteur commented on October 8th 2017 Owner
@tsteur commented on October 8th 2017 Owner

BTW: It seems to work fine when using eg ng-change="$ctrl.onRangeInputChanged('from', $event)" . I also tried ng-paste="$ctrl.onRangeInputChanged('to', $event)" in case browser stack pastes the values but this did not work.

@diosmosis commented on October 8th 2017 Member

@tsteur I can reproduce the backspace weirdness. I can also reproduce it on 3.x-dev. From the gif, it looks like when the selector fails to update nothing is happening in the console. Maybe the events aren't making it to the browserstack server, because of some local => browserstack network lag issue (of course, I have no way to check)? Can you check if the same issues exist in the calendar for 3.x-dev? If so, I think this might be browserstack related.

As for ng-change, the current implementation allows pressing enter in an input to select a period, so there has to be a keyup/down. (I tested in browserstack IE 10 and for some strange reason, the 'enter' behavior works w/ ng-change (despite throwing an "$event is undefined" exception when $event.keyCode is run), however this doesn't work in chrome.)

@tsteur commented on October 9th 2017 Owner

I noticed the same backspace problem there as well, however, the date was always updated correctly when I change it in the text field.

@diosmosis commented on October 9th 2017 Member

@tsteur Still can't reproduce it. Tried setting the timezone on my IE10 VM and it still works. If you could provide the console.log output from this comment: https://github.com/piwik/piwik/pull/11873#issuecomment-335042593 when triggering the bug that isn't visible in the 3.x-dev branch, then I might be able see what's happening.

Otherwise, since I can't reproduce this either in browserstack or my local VM, I will leave it to you & @mattab regarding if/how to move forward w/ this PR.

@tsteur commented on October 9th 2017 Owner

The thing is, it seems to not call this method when the bug appears and the console log does not appear therefore. I reckon we can otherwise just ignore it for now.

@diosmosis commented on October 9th 2017 Member

I'm not sure how important it is to keep investigating this, but this bug is really confusing me, so I'm gonna post a clarifying comment. Doesn't need a followup if it's not important.

The thing is, it seems to not call this method when the bug appears and the console log does not appear therefore. I reckon we can otherwise just ignore it for now.

The setDateCellColor() function (the position of the second console.log), should definitely get called if something gets selected in the browser. That's the function that actually adds the CSS class that makes the selected date have the black background & white foreground. So if a date is selected (including the wrong one), that function had to be executed. If the logs don't show up in the console then I can only assume browserstack is doing something funky.

@tsteur commented on October 9th 2017 Owner

See https://github.com/piwik/piwik/pull/11873#issuecomment-335044532

My problem is that it now doesn't even update the date at all in most of the cases. When I remove a number to 2017-10-0, then add a number to eg 2017-10-05 it doesn't not updated the selected field and the key-up event seems to be not triggered. Only when I then type another key to make it kind of 2017-10-055 it updates the field correctly and selects 2017-10-05. See this gif where I log dateStr in onRangeInputChanged()

            var dateStr = source === 'from' ? vm.startDate : vm.endDate;
            console.log(dateStr);

oct-09-2017 17-57-48

@tsteur commented on October 9th 2017 Owner

Maybe eg vm.endDate is only updated after keyUp sometimes?

@diosmosis commented on October 9th 2017 Member

Maybe eg vm.endDate is only updated after keyUp sometimes?

I see, that must be it! I'll split the events up.

@diosmosis commented on October 9th 2017 Member

@tsteur Pushed another commit, let me know if the browserstack bugs are still there.

@tsteur commented on October 9th 2017 Owner

It looks like it works now 👍

@mattab good to merge for me

@mattab commented on October 10th 2017 Owner

Looks good! :+1:

Feedback: (can be moved to another PR in case we merge it already today)

  • the tooltip that reads Click again to check this period is displayed too often I think (haven't looked at the code). the tooltip should be added on the period radio button, after it was clicked, to invite user to "click again once" to select the period. I think it should read instead Click again to apply this period. (currently the tooltip seems displayed on all period but it should be only displayed on one after it was clicked once already)
  • The feature of double clicking on the period works a bit different: to apply any period you can just double click twice on a period label (currently it looks like it requires a double click quite fast rather than two clicks at any interval)
  • using with the keyword should be possible (the select for year and month should be accessible with tab key, using the tabindex

    • (note that for date range input fields are already usable with the keyboard, but not the select fields yet. so users can first use the text fields which are more practical in this case the tabindex order could be: input-from, input-to, calendar-select-from, calendar-select-to, period checkbox, apply button)
  • when a month is selected and we're looking at another month, wondering if we could remove the black squares for days from the next/previous selected month:

    period select black no days

  • when we input an invalid date in the date range inputs, can we show a message or highlight the wrong date in some way (currently when applying an invalid date range, it closes the calendar but does not do anything.)
@tsteur commented on October 10th 2017 Owner

when we input an invalid date in the date range inputs, can we show a message or highlight the wrong date in some way (currently when applying an invalid date range, it closes the calendar but does not do anything.)

this is already currently the case

when a month is selected and we're looking at another month, wondering if we could remove the black squares for days from the next/previous selected month:

I found this is a useful feature to visually see the next month or next week is selected

The feature of double clicking on the period works a bit different: to apply any period you can just double click twice on a period label (currently it looks like it requires a double click quite fast rather than two clicks at any interval)

I am finding this much better now. So irritating how this was implemented before as you are usually trained to do either single or double clicks, but no weird two clicks with long break and suddenly you cannot even explain why it was selected etc

@mattab commented on October 10th 2017 Owner

I'm not seeing the feedback when the date is invalid eg.
no feedback

I found this is a useful feature to visually see the next month or next week is selected

Let's leave it (as it provides some value)

I am finding this much better now. So irritating how this was implemented before as you are usually trained to do either single or double clicks, but no weird two clicks with long break and suddenly you cannot even explain why it was selected etc

I think web apps these days don't really use double clicks. it's quite hard for some people to double click something (of course they can click the Apply button in that case). Where is the double click defined? could we increase the time between two clicks to allow a slower double click...

If we keep the double click we should rename the tooltip to Double click to apply this period

@diosmosis commented on October 10th 2017 Member

I think web apps these days don't really use double clicks. it's quite hard for some people to double click something (of course they can click the Apply button in that case). Where is the double click defined? could we increase the time between two clicks to allow a slower double click...

If we keep the double click we should rename the tooltip to Double click to apply this period

For context (as in, I don't have a strong opinion either way), I think the original logic for this is here: https://github.com/piwik/piwik/blob/3.x-dev/plugins/CoreHome/javascripts/calendar.js#L551

It's not really a double click, so much as a "if a user clicks on the currently selected period that isn't the period in the URL, change the period". There are two dblclick handlers above this code, but I don't think they're ever executed.

@tsteur commented on October 10th 2017 Owner

it's quite hard for some people to double click something (of course they can click the Apply button in that case). Where is the double click defined? could we increase the time between two clicks to allow a slower double click...

Users define double click speed in their operating system to a value they are comfortable with. I reckon dblclick they already need to use for many things, for example to open a file in an explorer or so. Most people will be able to double click :)

I don't have a strong opinion here but for good UX it would be best to not re-invent the wheel of a double click and to always behave consistent (which is not the case when you click once on a period, and then maybe a couple of seconds later again... you would not expect to select the period as the same action performed before did not do this)

@diosmosis commented on October 12th 2017 Member

@mattab any decision vs double click vs "click again"?

@mattab commented on October 12th 2017 Owner

I would def prefer "click again" but maybe we can solve the issue of clarifying that the second click will do something different from the first. For example, after clicking once on a period, we could maybe underline like this or so?
underline

@diosmosis commented on October 12th 2017 Member

To me that makes it seem like its the active period, ie, the period in the URL. What about a hover effect? Though this might be confusing too since you won't see until you click once, then all of a sudden you'll see the effect.

@mattab commented on October 12th 2017 Owner

Sounds good to try hover + tooltip

@tsteur commented on October 12th 2017 Owner

From a UX point of view, this behaviour will be always confusing as it won't be natural to users and often they won't be able to explain what just happened or an action executed they did not intend to :)

@diosmosis commented on October 12th 2017 Member

The more I think about it the more I think @tsteur is right. We're basically changing what an action does based on a slight context change. It's bound to be confusing if you're not prepared for it. Maybe we could introduce an entire new action, eg, a tiny icon next to the active period, like https://material.io/icons/#ic_launch ?

@mattab commented on October 12th 2017 Owner

Not sure about the new icon. It'd be ok to go with the double click for now, we can always re-evaluate later... :+1:

@diosmosis commented on October 12th 2017 Member

Made the following changes:

  • change the tooltip from 'click again' to 'double click'
  • removed jquery UI & period selector tabindexes > 1 (since they disrupt tabindexes throughout app)
  • use materializecss' "invalid" CSS class when invalid dates are entered into the range picker

Did another set of quick tests on chrome, firefox & ie10.

@mattab commented on October 12th 2017 Owner

Feedback

  • When a period is clicked once, the tooltip is not displayed but should be (since it still requires a double click)
  • when a date is invalid in the custom date range (and is highlighted in red), currently "Applying" the date range "works" (but in reality the wrong date or maybe previous date will be used). Instead, we need to prevent user from applying the change and leave the calendar opened, so user will notice something does not work. Even better maybe we could mark the button as disabled to make it more clear?
  • When pressing d (shortcut for date) it opens the calendar. Then pressing "tab" key should tab the user into the calendar (the first month select, or the date-from input field). Instead it currently tabs to the Segment editor control which makes it impossible to use calendar with keyboard.
@diosmosis commented on October 12th 2017 Member

Nice catches! will get to these today or tomorrow.

@diosmosis commented on October 13th 2017 Member

Applied the changes.

When a period is clicked once, the tooltip is not displayed but should be (since it still requires a double click)

Note: when you click the input you'll have to move the mouse off the label and back on to see the tooltip (ie, you must hover again).

Even better maybe we could mark the button as disabled to make it more clear?

if the range is invalid, the button is now disabled.

When pressing d (shortcut for date) it opens the calendar. Then pressing "tab" key should tab the user into the calendar (the first month select, or the date-from input field). Instead it currently tabs to the Segment editor control which makes it impossible to use calendar with keyboard.

fixed, though I think there are other places in piwik that use tabindex > 1

@diosmosis commented on October 13th 2017 Member

Noticed the tabindexes are coming from top_controls.js, will probably make another change to this pr

@diosmosis commented on October 15th 2017 Member

Tweaked the commit for tabindexes, ready for another review.

@mattab commented on October 16th 2017 Owner

This looks great!! :boom:

This Pull Request was closed on October 16th 2017
Powered by GitHub Issue Mirror