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
Convert period selector to angular & allow plugins to add periods to the frontend #11873
Conversation
Hi @diosmosis |
The code should be ready for review, it's just the screenshot that conflicts. |
I'll rebase this PR tomorrow. |
@diosmosis I didn't review the code in detail but noticed this:
I am seeing those errors in console: 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 |
@tsteur Interesting, I remember fixing these, maybe I broke something w/ the rebase... |
…just move, no refactoring).
… specific functionality to new extendable periods service.
… variable name in date range picker template.
…r directive translations and make sure periods can be extended in the frontend.
…ontext (ie, in embedded dashboard).
…it is now managed by angular.
…cting, even if loading a new page.
…s to ensure certain styles work properly.
…iod, even if loading a new page or changing the URL.
…ed & selected period date range stops being highlighted immediately when a range period is selected, even if loading a new page.
It looks like it works now 👍 @mattab good to merge for me |
Looks good! 👍 Feedback: (can be moved to another PR in case we merge it already today)
|
this is already currently the case
I found this is a useful feature to visually see the next month or next week is selected
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 |
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. |
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 any decision vs double click vs "click again"? |
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. |
Sounds good to try hover + tooltip |
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 :) |
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 ? |
Not sure about the new icon. It'd be ok to go with the double click for now, we can always re-evaluate later... 👍 |
Made the following changes:
Did another set of quick tests on chrome, firefox & ie10. |
Feedback
|
Nice catches! will get to these today or tomorrow. |
Applied the changes.
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).
if the range is invalid, the button is now disabled.
fixed, though I think there are other places in piwik that use tabindex > 1 |
Noticed the tabindexes are coming from top_controls.js, will probably make another change to this pr |
…riod is selected period.
…x case when \$.datepicker.parseDate returns null instead of throwing.
Tweaked the commit for tabindexes, ready for another review. |
This looks great!! 💥 |
This PR is based off #11857
Changes
Added
generate:angular-component
command to generate an empty angular component. Also modified the result ofgenerate: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 textChanged behavior
Removed behavior