feat(sort): show sort indicator on hover/focus/longpress #7608

andrewseguin · 2017-10-06T22:16:17Z

Brings the sort header closer to spec but unfortunately takes away the cool animation that was added in #7180. Not sure how to reconcile the animation with this indicator hint. Perhaps only do the fade-away animation when sort is removed but not added?

Closes #6858, #8612

crisbeto

I think that we can still keep the animation if we toggle the indicator visibility either through visibility or display.

jelbourn · 2017-10-08T15:42:55Z

I don't quite follow why showing the indicator means the animation can't exist

andrewseguin · 2017-10-13T18:32:36Z

This is only in reference to the animation we added for entering and exiting the sort (not transitioning between asc and desc).

Since the arrow shows on hover/focus, I don't think it's correct to show the arrow "fly in" since it is already in place when activated. Maybe we can have the arrow still "fly out" since the hint doesn't appear after the sort is disabled, but it wouldn't be a symmetrical animation (why does it fly out but never flies in)

jelbourn · 2017-10-26T06:18:49Z

I think I need to see it in action to understand

andrewseguin · 2017-10-27T19:23:42Z

Tried again for a few hours trying to get the animation working, its...more complex than it seems. Let's bring it back in another PR

jelbourn · 2017-10-27T20:27:38Z

@andrewseguin can you give a brief summary of why the animation is hard to keep?

andrewseguin · 2017-10-31T18:53:44Z

Had trouble with the animation out - the indicator wants to animate to the next state since its not active anymore but we want to preserve what state it was in as it animates out. Then, if you sort it while its animating out, you want it to float in from the bottom with that same position. I tried getting it to remember its old state after sort is removed, but it just got too tricky to add in this PR. I suspect it's totally doable but making it a clean implementation will take some thought

jelbourn · 2017-11-03T20:53:08Z

@crisbeto thoughts? I don't want to remove the animation "temporarily" and then never get around to bringing it back

crisbeto · 2017-11-03T21:15:37Z

I'm still having a hard time understanding the issue. Is it that we want to avoid the "fly in" animation when the user hovers the header?

andrewseguin · 2017-11-14T19:28:46Z

Discussed in person - issue is in its complexity and not clear what is the best animation for some cases.

Complexity: Need to watch several states - if active, arrow should be pointing in its current direction; if not active, arrow should be pointing in the start position except if the sort has just been disable (should fly out so the sort direction should persist)

andrewseguin · 2017-11-28T20:27:39Z

Ready again for review - after a looong time playing with the animations, found a way to bring back the animation. Essentially the sort will define the right transition it should show (e.g. go from above to the middle, asc-to-active)

jelbourn · 2017-11-28T21:58:54Z

src/lib/sort/sort-header.ts

+    // Arrow opacity
+    trigger('arrowOpacity', [
+      state('desc-to-active, asc-to-active, active',
+          style({opacity: 1})),


Why the line break?

Just consistency with the third state ('hint-to-desc, active-to-desc, desc, hint-to-asc, active-to-asc, asc') which causes wraparound. No strong opinion either way - I unwrapped it

jelbourn · 2017-11-28T23:12:05Z

src/lib/sort/sort-header.ts

 import {MatSortHeaderIntl} from './sort-header-intl';
 import {getSortHeaderNotContainedWithinSortError} from './sort-errors';
 import {AnimationCurves, AnimationDurations} from '@angular/material/core';

 const SORT_ANIMATION_TRANSITION =
    AnimationDurations.ENTERING + ' ' + AnimationCurves.STANDARD_CURVE;

+/**
+ * Valid positions for the arrow to be in for its opacity and translation.


Expand comment to explain what 'peek' and 'active' mean?

jelbourn · 2017-11-28T23:13:46Z

src/lib/sort/sort-header.ts

+   * provide an affordance that the header is sortable by showing on focus and hover.
+   * @docs-private
+   */
+  set showIndicatorHint(showIndicatorHint: boolean) {


Instead of making this a setter, can you make it a function like _setIndicatorHintVisible?

jelbourn · 2017-11-28T23:14:41Z

src/lib/sort/sort-header.ts

+   * so that there is no animation displayed.
+   * @docs-private
+   */
+  set viewState(arrowPosition: ArrowViewStateTransition) {


Also make this a function? Something like _setAnimationTransitionState?

jelbourn · 2017-11-28T23:16:13Z

src/lib/sort/sort-header.ts

+   * facing the start direction. Otherwise if it is sorted, the arrow should point in the currently
+   * active sorted direction.
+   */
+  _updateArrowDirection() {


Why have an update function instead of a function like _getArrowDirection? The problem with the update function is that you can forget to call it, or call it when it's not necessary.

I tried going that route but this ended up being much easier to control when the arrow changes directions.

Best example is when the sort goes from active to deactivated. By default the order for sort will be none - asc - desc. When the sort goes from desc to none, the arrow direction should remain the same until the animation completes (as it flies away). Only when the arrow is displayed again should it switch to showing itself as asc (hint is shown or activated). With just a "getter", we would probably have to rely on knowing if the arrow is animating away and have some additional logic about making it keep the "old direction" in the meantime.

Expand comment to explain this, then?

jelbourn · 2017-11-28T23:16:49Z

src/lib/sort/sort.spec.ts

@@ -88,6 +93,95 @@ describe('MatSort', () => {
    expect(sortables.has('column_c')).toBe(true);
  });

+  describe('checking correct arrow direction and view state for its various states', () => {
+    let expectedStates: Map<string, { viewState: string, arrowDirection: string }>;


Omit spaces in braces

jelbourn · 2017-11-28T23:20:32Z

src/lib/sort/sort.spec.ts

+    dispatchMouseEvent(sortElement, event);
+  }
+
+  expectViewAndDirectionStates(


Add JsDoc description? It's not super obvious what this method does

jelbourn

LGTM

tinayuangao · 2017-12-01T18:33:37Z

@andrewseguin This has error _disableStateAnimation not exist.

andrewseguin · 2017-12-01T21:59:09Z

Rebased -- should be alright now

andrewseguin · 2018-01-18T23:02:08Z

Woohoo, got it working!

jelbourn · 2018-01-19T01:49:37Z

@andrewseguin get a presubmit going?

angular-automatic-lock-bot · 2019-09-08T18:03:10Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

andrewseguin added mat-table labels Oct 6, 2017

andrewseguin requested review from jelbourn and crisbeto October 6, 2017 22:16

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 6, 2017

crisbeto reviewed Oct 7, 2017

View reviewed changes

andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Oct 20, 2017

andrewseguin removed the mat-table label Oct 31, 2017

andrewseguin force-pushed the sort-display-on-hover branch 2 times, most recently from 48f4152 to 7473ce9 Compare November 20, 2017 22:46

andrewseguin force-pushed the sort-display-on-hover branch from 7473ce9 to 4d9caff Compare November 28, 2017 20:23

andrewseguin added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Nov 28, 2017

andrewseguin added this to the 5.0 milestone Nov 28, 2017

jelbourn reviewed Nov 28, 2017

View reviewed changes

jelbourn approved these changes Nov 29, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 29, 2017

andrewseguin force-pushed the sort-display-on-hover branch from 624d1c7 to ed1b956 Compare December 1, 2017 21:58

andrewseguin added target: minor This PR is targeted for the next minor release presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Dec 11, 2017

feat(sort): fix animation; show on hover

a358972

andrewseguin force-pushed the sort-display-on-hover branch from ed1b956 to a358972 Compare January 18, 2018 23:01

tinayuangao merged commit cde00df into angular:master Jan 31, 2018

andrewseguin deleted the sort-display-on-hover branch April 17, 2018 17:24

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

feat(sort): show sort indicator on hover/focus/longpress #7608

feat(sort): show sort indicator on hover/focus/longpress #7608

Uh oh!

Conversation

andrewseguin commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Oct 8, 2017

Uh oh!

andrewseguin commented Oct 13, 2017

Uh oh!

jelbourn commented Oct 26, 2017

Uh oh!

andrewseguin commented Oct 27, 2017

Uh oh!

jelbourn commented Oct 27, 2017

Uh oh!

andrewseguin commented Oct 31, 2017

Uh oh!

jelbourn commented Nov 3, 2017

Uh oh!

crisbeto commented Nov 3, 2017

Uh oh!

andrewseguin commented Nov 14, 2017

Uh oh!

andrewseguin commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Dec 1, 2017

Uh oh!

andrewseguin commented Dec 1, 2017

Uh oh!

andrewseguin commented Jan 18, 2018

Uh oh!

jelbourn commented Jan 19, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!

andrewseguin commented Oct 6, 2017 •

edited

Loading