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

Tabledrag hierarchy is broken in Basis #2689

Open
docwilmot opened this issue May 14, 2017 · 13 comments
Open

Tabledrag hierarchy is broken in Basis #2689

docwilmot opened this issue May 14, 2017 · 13 comments

Comments

@docwilmot
Copy link
Contributor

Steps to reproduce

  • Create a multilevel hierarchical vocabulary (generate some tags and drag/drop to make some child/parent levels)
  • Change your admin theme to Basis and go back to that tags list
  • Note that it is now displayed will all terms at root level and you cannot drag tags laterally, only up and down
@docwilmot docwilmot added this to the 1.6.4 milestone May 15, 2017
@quicksketch quicksketch modified the milestones: 1.6.4, 1.7.1 May 15, 2017
@docwilmot
Copy link
Contributor Author

docwilmot commented May 17, 2017

I've tracked this down to one line in Basis css/base.css: line 6:

* {
  box-sizing: border-box;
}

Commenting this out enables lateral-movement tabledrag again. But even though the rows move laterally, the tabledrag handle stays fixed to the left table edge because Basis also has a position: absolute; on the handle CSS in components/tabledrag.css line 56.

tabledrag

So in summary, two things to fix this: the box-sizing, and the drag-handle positioning. I am not sure what those fixes should be. Tagging @wesruv.

@serundeputy
Copy link
Member

I agree with your conclusions here @docwilmot My CSS foo is not strong enough for this.

@wesruv is there someway we can scope those kinds of rules from base.css and tabledrag.css to only apply if the user is on the front end i.e. if the url is of the form /admin/% then these rules should not fire?

@quicksketch quicksketch modified the milestones: 1.7.1, 1.7.2 Jun 21, 2017
@jenlampton jenlampton modified the milestones: 1.7.2, 1.7.3 Aug 17, 2017
@jenlampton jenlampton modified the milestones: 1.7.3, 1.8.1 Sep 16, 2017
@quicksketch quicksketch modified the milestones: 1.8.1, 1.8.2 Jan 2, 2018
@jenlampton jenlampton removed this from the 1.8.2 milestone Jan 15, 2018
@indigoxela
Copy link
Member

What would be the desired result here, what should this look like? Proper indentation, for sure, but what else? A quick mockup could help.

@indigoxela
Copy link
Member

indigoxela commented Mar 21, 2021

Here's a PR as a starting point for further discussion.
Currently it only fixes indentation.

I expect this issue to be a tricky one, as fixing the hierarchy handling might need deeper changes - potentially problematic ones regarding existing subthemes out there.

A screenshot of what we have so far:
term-hierarchy-basis

@docwilmot
Copy link
Contributor Author

Seen; I made a term list at http://3572.backdrop.backdrop.qa.backdropcms.org/admin/structure/taxonomy/draggable for testing.

I expect this issue to be a tricky one ... regarding existing subthemes out there.

If it never worked, I think we should fix it if we can.

@klonos
Copy link
Member

klonos commented Mar 21, 2021

I expect this issue to be a tricky one...

Yeah. It seems that the current design did not take nested table rows into consideration. The drag handler should be immediately next to each item. Here's how things look with the Seven admin theme in Backdrop for reference:

image

...and in D8 with Claro:

image

@indigoxela
Copy link
Member

It seems that the current design did not take nested table rows into consideration. The drag handler should be immediately next to each item.

I agree, this is the main problem here. Moving the drag handler next to the items contradicts the design concept and might look awkward. ... And needs a lot of rewrite.

@indigoxela
Copy link
Member

Before we re-invent the wheel... maybe we can circumvent the problem of a bigger design change with more significant indent indication?

I pushed an experimental (and by no means elaborated!) approach to my PR. Not sure how good the UX is.

A screenshot with a hierarchy:
term-hierarchy-basis-2

@wesruv
Copy link
Member

wesruv commented Mar 25, 2021

I can take a look this week

@wesruv
Copy link
Member

wesruv commented Mar 29, 2021

Made a PR backdrop/backdrop#3578

The CSS for tabledrag is gnarly, it could be so much simpler if the indentation divs were added before the tabledrag link, but that's not how it works 😭

I simplified the look, which is a little sad, but we'd need much better JS/DOM order for me to have fancy styles like I did.

image

@indigoxela
Copy link
Member

@wesruv Thank you so much for digging into this. 👍

Two minor issues:

  1. The tree indication when moving a part of the hierarchy seems broken (the lines that indicate parent/child relations don't fully show up when moving a part of the tree (LTR))
  2. The RTL hierarchy isn't showing up yet, they're aligned right without indentation

Otherwise it's looking good. Yes, it's a bit sad that the fancy style had to be dropped in favor of proper hierarchy display...

@klonos
Copy link
Member

klonos commented Mar 29, 2021

it's a bit sad that the fancy style had to be dropped in favor of proper hierarchy display

Sad, but necessary. In the previous design we seem to have overlooked the use case of hierarchical tabledrag 🤷🏼

@indigoxela
Copy link
Member

After years without additional feedback and because CSS changes are known to be hard, I'm closing my PR. No need for another PR queue zombie. 😆

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

Successfully merging a pull request may close this issue.

8 participants