List: Sortable list interactions update #1541

philschanely · 2022-08-01T20:35:38Z

Description

This PR updates styling for interactions with sortable List.

Screenshots

Before	After

Testing in `sage-lib`

See Rails http://localhost:4000/pages/component/list?tab=preview
See React http://localhost:4100/?path=/story/sage-list--sortable-list

Testing in `kajabi-products`

(LOW) Updates interactive styling for the List (sortable) component in Rails and React.

Actually clicking and dragging on the drag icon doesn't work for me in FF

Does work in Chrome

As Chris noted, the button in Firefox (and to a lesser extent, Safari when "holding" the button for a second or so) does not propagate the parent list item's event.

Whether or not the icon ends up being the drag "trigger" rather than the list item, we should consider changing it back to an icon rather than a true button element. This would at least restore the draggable behavior in FF/Safari and also be somewhat less confusing to AT users, since the label indicates that the button allows sorting, but it's only possible with a mouse.

pixelflips

Looks and works great! LGTM! Nice work! 👍🏼

goodwinchris · 2022-08-04T14:22:26Z

Actually clicking and dragging on the drag icon doesn't work for me in FF

Does work in Chrome

teenwolfblitzer

👍🏼 As it currently stands, the functionality is not keyboard accessible, but this is part of the existing behavior, and not necessarily a blocker.

On a related note, the accessible capabilities of Sortable JS appear to be lacking. I'm wondering if we can replace it with a custom solution using the draggable API for a future follow-up. Or maybe leverage something like this as a shortcut.

Last thing… the sortable_update_url config in the Rails preview throws an error when onEnd fires. If we're not storing that state, should we remove it from the preview?

teenwolfblitzer · 2022-08-08T23:41:22Z

docs/lib/sage_rails/app/views/sage_components/_sage_list_item.html.erb

@@ -19,7 +19,12 @@ end
 >
  <% if component.sortable %>
    <div class="sage-list__item-sortable-handle">
-      <%= sage_component SageIcon, { icon: "handle" } %>
+      <%= sage_component SageButton, {


Actually clicking and dragging on the drag icon doesn't work for me in FF

Does work in Chrome

As Chris noted, the button in Firefox (and to a lesser extent, Safari when "holding" the button for a second or so) does not propagate the parent list item's event.

Whether or not the icon ends up being the drag "trigger" rather than the list item, we should consider changing it back to an icon rather than a true button element. This would at least restore the draggable behavior in FF/Safari and also be somewhat less confusing to AT users, since the label indicates that the button allows sorting, but it's only possible with a mouse.

cameronsimony

paired with @philschanely for design QA on this & it looks great! a big 👍 from me!

philschanely · 2022-08-18T02:37:37Z

@goodwinchris does this work better/consistently for you now?

@monkeypox8 are the changes here to your liking as well?

philschanely · 2022-08-18T15:36:08Z

@monkeypox8 @pixelflips requesting a fresh review after having updated this PR.

pixelflips

A couple of minor non-blocking comments. In Safari, when dragging an item in the top section, items in the bottom section get highlighted/a background color applied.

list-highlight.mov

pixelflips · 2022-08-18T15:49:20Z

docs/app/views/examples/components/list/_preview.html.erb

@@ -89,7 +131,7 @@ If you need more native content formatting you can instead opt to render items u
      css_classes: SageClassnames::REVEAL_CONTAINER,
    } do %>
      <%= sage_component SageCardRow, { grid_template: "ete" } do %>
-        <img src="//source.unsplash.com/random/240x160?v=<%= item[:id] %>" width="100" alt="" />
+        <img src="//source.unsplash.com/random/240x160?v=<%= item[:id] %>" width="64" alt="" />


Def not a blocker, but could we add the protocol to the image src here? Protocol-relative URLs have become somewhat of an antipattern now that SSL is highly encouraged and HTTPS no longer has performance issues.

pixelflips · 2022-08-18T15:51:23Z

packages/sage-react/lib/List/List.jsx

@@ -64,7 +102,6 @@ List.Item = ListItem;
 List.defaultProps = {
  children: null,
  className: null,
-  hideFirstBorder: false,


If hideFirstBorder is no longer needed, would be worth removing it from the Rails side and the related styles as well? Could definitely be handled in a follow-up

pixelflips · 2022-08-18T15:53:47Z

packages/sage-system/lib/list.js

+      sortableConfigs = { ...DEFAULT_CONFIGS };
+    }
+
+    console.log('final configs', resourceName, sortableConfigs);


Should the console.log be removed?

pixelflips

Rechecked and am no longer seeing the ID issue as we discussed. LGTM! 👍🏼

teenwolfblitzer

👍 LGTM, nice work! Thanks for getting this… sorted out 😝

ju-Skinner · 2022-08-19T13:50:04Z

docs/lib/sage_rails/app/views/sage_components/_sage_list.html.erb

@@ -5,16 +5,16 @@ tag = component.tag.present? ? component.tag : "ul"
  class="
    sage-list
    <%= component.generated_css_classes %>
-    <%= "sage-list--hide-first-border" if component.hide_first_border %>
+    <%= "sage-list--all-draggable" if component.sortable_configs && component.sortable_configs[:handle] == false %>


ruby tip you can change this to component.dig(:sortable_configs, :handle).blank?. This will account for it either being nil or false'. Handle being set to true will pass the blank? check.

[7] pry(main)> component = {sortable_configs: {handle: false}} => {:sortable_configs=>{:handle=>false}} [8] pry(main)> component.dig(:sortable_configs, :handle).blank? => true [9] pry(main)> component.dig(:sortable_configs, :handle) => false [10] pry(main)> component = {sortable_configs: {handle: true}} => {:sortable_configs=>{:handle=>true}} [11] pry(main)> component.dig(:sortable_configs, :handle).blank? => false [12] pry(main)>

ju-Skinner · 2022-08-19T13:56:20Z

Lint failure

packages/sage-assets/lib/stylesheets/components/_table.scss
 35:5  ⚠  Expected indentation of 2 spaces   indentation

philschanely · 2022-08-22T20:13:43Z

@monkeypox8 @pixelflips and @ju-Skinner since last review I made the following updates to further hone how this component works:

I abandoned the sortableConfigs/sortable_configs prop that was initially meant to allow the user to pass configs down to the SortableJS inside the component. On React, I instead surfaced only the props we intend to expose at this time: setList, onStart, and onEnd. On Rails this is not practical at this time (and not attempted in previous Sortable component either).
I added a dragHandleType/drag_handle_type prop that allows user to choose between "default" and "row". The default is to only allow the end user to sort/drag by the drag handle, but "row" makes the whole row draggable.
I moved sortable<boolean> as a prop up from the children Items to the parent List.

pixelflips

Rechecked all main browsers and all still LGTM! 👍🏼

teenwolfblitzer

👍🏼 LGTM! Two final items to consider; both are documentation-related so not considered as blockers for the merge.

Noting here also that I'm not able to get the bridge running due to a bundler issue.

teenwolfblitzer · 2022-08-22T22:22:12Z

docs/app/views/examples/components/list/_preview.html.erb

+", use_sage_type: true) %>
+
+<%= sage_component SageList, {
+  sortable: true,


For the sortable examples, thoughts on adding tag: "ol" to drive home the ordinal values?

I suppose we could also be more heavy-handed and change the default to an ol when setting sortable: true in the component, but that's beyond the scope of this work.

teenwolfblitzer · 2022-08-22T22:34:41Z

packages/sage-react/lib/List/List.story.jsx

+    ...selectArgs({
+      dragHandleType: List.DRAG_HANDLE_TYPES,
+    }),
+  },
  args: {
    items: [],


I'm getting an error when attempting to toggle the base list to "sortable" in Storybook. Seems it's because we're not defining setList for the default config?

That's right. The setList needs to be set as well so it's not as straightforward to switch props in storybook.

pixelflips

On drag-n-drop I am seeing the following error in the console. I only point it out as I am seeing this popping up in DataDog UX Monitor Errors on the KP side as well.

Definitely not a blocker, since it looks like it's been hanging around for a while.

pixelflips

It may or may not be related to these updates, but I seem to be getting some odd behavior when clicking the dropdown in the fully draggable section. After opening the dropdown, clicking out of it kinda blinks and doesn't close smoothly if at all.

Screen.Recording.2022-08-22.at.4.56.46.PM.mov

ju-Skinner

see comments. the request changes is for documentation purposes in docs/app/views/examples/components/list/_preview.html.erb. There are a couple of other callouts as well.

ju-Skinner · 2022-08-23T13:25:29Z

docs/app/views/examples/components/list/_preview.html.erb

+<h3 class="<%= SageClassnames::TYPE::HEADING_6 %>">Fully draggable row</h3>
+<%= md("
+By default only the drag handle is active for dragging/sorting a row.
+However, `draggable_anywhere` can be set to `true` in order to allow the whole row to be draggable instead.


I believe you need to change draggable_anywhere to "drag_handle_type can be set to row" here.

ju-Skinner · 2022-08-23T13:29:54Z

docs/lib/sage_rails/app/views/sage_components/_sage_list.html.erb

@@ -1,11 +1,13 @@
 <%
 tag = component.tag.present? ? component.tag : "ul"
+drag_handle_type = component.tag.present? ? component.tag : "default"


This may have been an oversight or a misunderstanding from me. drag_handle_type accepts two options based on the code above default and row. So if component.tag is anything other than those two values, what would be the expectation here?

Is this tag: [:optional, NilClass, Set.new(["ul", "ol"])], the same component.tag values?

Oh wow good catch. I'll touch this up for sure!

ju-Skinner · 2022-08-23T13:31:30Z

packages/sage-react/lib/List/List.jsx

  items: [],
  itemRenderer: null,
-  sortableConfigs: null,
+  dragHandleType: List.DRAG_HANDLE_TYPES.DEFAULT,
+  onEnd: null,


for functions it is recommended to use noop vs null

Hmm... validation does not like that... is it an accepted keyword in js?

I set these up as () => null but lmk if that's not what you were looking for.

Sorry, I made a bad assumption. So noop is a lodash function, that essentially returns undefined which is completely different than the value of null.

Examples taken from console....created a noop function to mimic lodash to show difference.

null === undefined false function noop() { return undefined } undefined null === noop() false undefined === noop() true

ju-Skinner · 2022-08-23T14:33:45Z

packages/sage-react/lib/List/List.jsx

-  onEnd: null,
-  onStart: null,
-  setList: null,
+  onEnd: () => null,


this is the equivalent to what you had before...you can do one of two things

() => {} // returns undefined <- this is common

() => ( undefined ) // explicitly returns undefined

docs/lib/sage_rails/app/views/sage_components/_sage_list.html.erb

ju-Skinner

Great job.

philschanely self-assigned this Aug 1, 2022

philschanely marked this pull request as ready for review August 3, 2022 12:04

philschanely requested review from a team and cameronsimony August 3, 2022 12:05

pixelflips reviewed Aug 3, 2022

View reviewed changes

pixelflips approved these changes Aug 3, 2022

View reviewed changes

pixelflips requested a review from a team August 3, 2022 16:30

teenwolfblitzer approved these changes Aug 8, 2022

View reviewed changes

philschanely force-pushed the SAGE-535/pjs-sortable-list-interactions-update branch from ad7cc03 to 8981b7a Compare August 17, 2022 14:39

ju-Skinner force-pushed the develop branch from 5e0a4bd to 7f9435d Compare August 17, 2022 20:40

cameronsimony approved these changes Aug 17, 2022

View reviewed changes

philschanely requested a review from a team August 18, 2022 02:37

philschanely force-pushed the SAGE-535/pjs-sortable-list-interactions-update branch from 8981b7a to ca7a647 Compare August 18, 2022 14:08

philschanely requested review from pixelflips and teenwolfblitzer August 18, 2022 15:35

pixelflips reviewed Aug 18, 2022

View reviewed changes

philschanely requested a review from pixelflips August 18, 2022 18:21

pixelflips approved these changes Aug 18, 2022

View reviewed changes

philschanely force-pushed the SAGE-535/pjs-sortable-list-interactions-update branch from abf1c7e to d06dec6 Compare August 18, 2022 19:12

teenwolfblitzer approved these changes Aug 18, 2022

View reviewed changes

philschanely requested a review from a team August 19, 2022 02:32

ju-Skinner reviewed Aug 19, 2022

View reviewed changes

ju-Skinner approved these changes Aug 19, 2022

View reviewed changes

philschanely added 2 commits August 19, 2022 14:02

style(list): update sortable interaction styles

7bd6207

feat(list): update list interaction (rails)

7e3d76f

philschanely added 7 commits August 19, 2022 14:02

feat(list): wip adding advanced drag features

e9c76d7

chore(list): convert button handle back to just icon

86c23e8

feat(list): add fallback and improved documentation

6b6f0ff

feat(lsit): cleanup

27237c5

feat(list): remove hide_first_border property that is no longer needed

9cc8253

chore(list): clean up linting and documentation exception

c98ad63

chore(list): clean up from feedback

608e608

philschanely force-pushed the SAGE-535/pjs-sortable-list-interactions-update branch from d06dec6 to 608e608 Compare August 19, 2022 18:02

feat(list): adjust handle affordance for clarity

f2f43db

philschanely requested review from teenwolfblitzer, pixelflips, ju-Skinner and a team August 22, 2022 20:13

chore: lint

d677c8a

pixelflips approved these changes Aug 22, 2022

View reviewed changes

teenwolfblitzer approved these changes Aug 22, 2022

View reviewed changes

pixelflips reviewed Aug 22, 2022

View reviewed changes

pixelflips reviewed Aug 23, 2022

View reviewed changes

ju-Skinner requested changes Aug 23, 2022

View reviewed changes

chore(list): clean up from feedback

14c6be0

philschanely requested a review from ju-Skinner August 23, 2022 13:59

ju-Skinner requested changes Aug 23, 2022

View reviewed changes

ju-Skinner reviewed Aug 23, 2022

View reviewed changes

docs/lib/sage_rails/app/views/sage_components/_sage_list.html.erb Outdated Show resolved Hide resolved

chore: adjust from feedback

62c7bf1

philschanely requested a review from ju-Skinner August 23, 2022 15:05

ju-Skinner approved these changes Aug 23, 2022

View reviewed changes

philschanely merged commit 71a5a82 into develop Aug 23, 2022

philschanely deleted the SAGE-535/pjs-sortable-list-interactions-update branch August 23, 2022 15:15

philschanely mentioned this pull request Aug 23, 2022

Next version bump #1568

Merged

4 tasks

List: Sortable list interactions update #1541

List: Sortable list interactions update #1541

Conversation

philschanely commented Aug 1, 2022

Description

Screenshots

Testing in sage-lib

Testing in kajabi-products

Related

philschanely commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teenwolfblitzer Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

pixelflips left a comment

Choose a reason for hiding this comment

goodwinchris commented Aug 4, 2022 • edited Loading

teenwolfblitzer left a comment • edited Loading

Choose a reason for hiding this comment

teenwolfblitzer Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

cameronsimony left a comment

Choose a reason for hiding this comment

philschanely commented Aug 18, 2022

philschanely commented Aug 18, 2022

pixelflips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelflips Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

pixelflips left a comment

Choose a reason for hiding this comment

teenwolfblitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju-Skinner commented Aug 19, 2022

philschanely commented Aug 22, 2022

pixelflips left a comment

Choose a reason for hiding this comment

teenwolfblitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelflips left a comment • edited Loading

Choose a reason for hiding this comment

pixelflips left a comment • edited Loading

Choose a reason for hiding this comment

ju-Skinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju-Skinner left a comment

Choose a reason for hiding this comment

Testing in `sage-lib`

Testing in `kajabi-products`

teenwolfblitzer Aug 8, 2022 •

edited

Loading

goodwinchris commented Aug 4, 2022 •

edited

Loading

teenwolfblitzer left a comment •

edited

Loading

teenwolfblitzer Aug 8, 2022 •

edited

Loading

pixelflips Aug 18, 2022 •

edited

Loading

pixelflips left a comment •

edited

Loading

pixelflips left a comment •

edited

Loading