Skip to content
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
Merged

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

merged 32 commits into from Oct 16, 2017

Conversation

diosmosis
Copy link
Member

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

@diosmosis diosmosis added c: Design / UI For issues that impact Matomo's user interface or the design overall. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jul 16, 2017
@sgiehl sgiehl added this to the 3.1.0 milestone Jul 17, 2017
@mattab
Copy link
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
Copy link
Member Author

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

@diosmosis
Copy link
Member Author

I'll rebase this PR tomorrow.

@tsteur
Copy link
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
Copy link
Member Author

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

… 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.
…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.
@tsteur
Copy link
Member

tsteur commented Oct 9, 2017

It looks like it works now 👍

@mattab good to merge for me

@mattab
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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

@mattab
Copy link
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
Copy link
Member Author

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
Copy link
Member

mattab commented Oct 12, 2017

Sounds good to try hover + tooltip

@tsteur
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

Nice catches! will get to these today or tomorrow.

@diosmosis
Copy link
Member Author

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
Copy link
Member Author

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

@diosmosis
Copy link
Member Author

Tweaked the commit for tabindexes, ready for another review.

@mattab
Copy link
Member

mattab commented Oct 16, 2017

This looks great!! 💥

@mattab mattab merged commit d4e5727 into matomo-org:3.x-dev Oct 16, 2017
@diosmosis diosmosis deleted the period-selector-angular branch October 16, 2017 05:39
@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
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants