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

Convert period selector to angular & allow plugins to add periods to the frontend #11873

Merged
merged 32 commits into from Oct 16, 2017

Conversation

Projects
None yet
4 participants
@diosmosis
Member

diosmosis commented Jul 16, 2017

This PR is based off #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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 11, 2017

Member

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?

Member

mattab commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 11, 2017

Member

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

Member

diosmosis commented Sep 11, 2017

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

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 13, 2017

Member

I'll rebase this PR tomorrow.

Member

diosmosis commented Sep 13, 2017

I'll rebase this PR tomorrow.

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 24, 2017

Member

@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.

Member

tsteur commented Sep 24, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Sep 26, 2017

Member

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

Member

diosmosis commented Sep 26, 2017

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

diosmosis added some commits Jul 11, 2017

Extract single period calendar to separate component & extract period…
… specific functionality to new extendable periods service.
Move bulk of period selector code from directive to controller, & fix…
… variable name in date range picker template.
Fix issue w/ yesterday date value, remove need to give period selecto…
…r directive translations and make sure periods can be extended in the frontend.
Make sure selected period text changes immediately after selecing per…
…iod, even if loading a new page or changing the URL.
Make sure range start/end changes immediately when a period is select…
…ed & selected period date range stops being highlighted immediately when a range period is selected, even if loading a new page.
@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 10, 2017

Member

Looks good! 👍

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.)
Member

mattab commented Oct 10, 2017

Looks good! 👍

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

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 10, 2017

Member

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

Member

tsteur commented Oct 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 10, 2017

Member

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

Member

mattab commented Oct 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 10, 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.

Member

diosmosis commented Oct 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 10, 2017

Member

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)

Member

tsteur commented Oct 10, 2017

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)

@mattab mattab modified the milestones: 3.2.0, 3.2.1 Oct 10, 2017

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 12, 2017

Member

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

Member

diosmosis commented Oct 12, 2017

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

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 12, 2017

Member

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

Member

mattab commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 12, 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.

Member

diosmosis commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 12, 2017

Member

Sounds good to try hover + tooltip

Member

mattab commented Oct 12, 2017

Sounds good to try hover + tooltip

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Oct 12, 2017

Member

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 :)

Member

tsteur commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 12, 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 ?

Member

diosmosis commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 12, 2017

Member

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

Member

mattab commented Oct 12, 2017

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

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 12, 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.

Member

diosmosis commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 12, 2017

Member

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.
Member

mattab commented Oct 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 12, 2017

Member

Nice catches! will get to these today or tomorrow.

Member

diosmosis commented Oct 12, 2017

Nice catches! will get to these today or tomorrow.

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 13, 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

Member

diosmosis commented Oct 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 13, 2017

Member

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

Member

diosmosis commented Oct 13, 2017

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

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Oct 15, 2017

Member

Tweaked the commit for tabindexes, ready for another review.

Member

diosmosis commented Oct 15, 2017

Tweaked the commit for tabindexes, ready for another review.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Oct 16, 2017

Member

This looks great!! 💥

Member

mattab commented Oct 16, 2017

This looks great!! 💥

@mattab mattab merged commit d4e5727 into matomo-org:3.x-dev Oct 16, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@diosmosis diosmosis deleted the diosmosis:period-selector-angular branch Oct 16, 2017

@matomo-org matomo-org deleted a comment from MatomoForumBot Dec 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment