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

Use new Icon API for Tabledrag multi-arrow icons #6493

Open
quicksketch opened this issue Apr 30, 2024 · 6 comments · May be fixed by backdrop/backdrop#4720
Open

Use new Icon API for Tabledrag multi-arrow icons #6493

quicksketch opened this issue Apr 30, 2024 · 6 comments · May be fixed by backdrop/backdrop#4720

Comments

@quicksketch
Copy link
Member

quicksketch commented Apr 30, 2024

Now that we have the new SVG icons from #364, we should use one for tabledrag:

image

@quicksketch quicksketch changed the title #xx | Tabledrag multi-arrow icons Use new Icon API for Tabledrag multi-arrow icons Apr 30, 2024
@quicksketch
Copy link
Member Author

Work-in-progress PR filed at backdrop/backdrop#4720

image

Although not strictly necessary, I added tabledrag.js to a library so it can be added (along with its icons) via backdrop_add_library() instead of backdrop_add_js(). I also moved its CSS outside of system.css, since it is not needed on most pages.

@indigoxela
Copy link
Member

indigoxela commented Apr 30, 2024

I added tabledrag.js to a library ... I also moved its CSS outside of system.css

Ouch, that doesn't sound like it's a backwards compatible move, though. At least one of my contrib modules and probably most of my themes will be affected by such a change. 😬

@argiepiano
Copy link

argiepiano commented Apr 30, 2024

Ouch, that doesn't sound like it's a backwards compatible move, though.

AFAIK you can still load tabledrag.js the "old" way with backdrop_add_js, despite it being added to a library, no? If not, several of my custom modules will also be affected...

@quicksketch
Copy link
Member Author

@indigoxela Yeah... I wonder if this is feasible as well; moving tabledrag CSS into its own file. Are you actually overriding system.css (not system.theme.css)? That's a big set of functional CSS to override.

@argiepiano From what I've seen in contrib modules, it's pretty unusual to add tabledrag.js directly, usually you call backdrop_add_tabledrag(), which is needed to add the corresponding settings to the page. That in turn adds the JS (or the new library in this case).

@indigoxela
Copy link
Member

indigoxela commented Apr 30, 2024

Are you actually overriding system.css (not system.theme.css)?

I'm overriding tabledrag styles in (almost?) every theme I create. Just CSS. But moving things around sounds really dangerous re backwards compatibility. Needs proper testing, as tabledrag's used in lots of module and probably CSS gets overridden in many (most) themes. Contrib or custom.

... usually you call backdrop_add_tabledrag()

Yes, that's what my module uses.

However, tabledrag is one of those things used in lots of modules. So my "bc alarm" got triggered. 😉

@klonos klonos added this to In progress in SVG support May 1, 2024
@klonos klonos added this to To do in SVG icons May 1, 2024
@klonos klonos moved this from To do to In progress in SVG icons May 1, 2024
@klonos klonos removed this from In progress in SVG support May 1, 2024
@klonos
Copy link
Member

klonos commented May 2, 2024

With regards to the icons:

  • consider increasing the contrast, as that light gray over white background seems insufficient (haven't actually tested though)
  • perhaps also use arrow-elbow-down-left/arrow-elbow-down-right icons from the Phosphor icon set instead of the custom tree.png and tree-bottom.png ones we currently use:
    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
SVG icons
In progress
Development

Successfully merging a pull request may close this issue.

4 participants