- Issue created by @Gauravvv
- Status changed to Needs review
over 1 year ago 12:44pm 31 January 2023 - ๐ฎ๐ณIndia Gauravvv Delhi, India
I have provided the patch for same, Please review
- ๐ฎ๐ณIndia Rinku Jacob 13 Kerala
Hi @Gauravvv, I have Reviewed the patch on drupal 10.1.x.The patch was successfully applied. Need RTBC+1
- Assigned to Guru2023
- Issue was unassigned.
- Status changed to RTBC
over 1 year ago 6:12am 3 February 2023 - ๐ฎ๐ณIndia Guru2023
Verified the patch #3 on Drupal version 10.1.x.
The patch works fine and I have added the before and after screenshot for reference. The last submitted patch, 3: 3337909-3.patch, failed testing. View results โ
- Status changed to Needs work
over 1 year ago 7:43pm 16 March 2023 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
This needs screenshots of use cases where indentation.html.twig is in use. Tabledrag, including situations where the tree lines are visible would work.
This does not (and never did) need screenshots of a patch applying or source code. The patches + testbot do that for us already.
- Status changed to Needs review
over 1 year ago 3:40am 17 March 2023 - ๐ฎ๐ณIndia Gauravvv Delhi, India
I have added before/after patch screenshots. please review
- Status changed to RTBC
over 1 year ago 2:09pm 17 March 2023 The last submitted patch, 3: 3337909-3.patch, failed testing. View results โ
- ๐ฎ๐ณIndia Akshay kashyap
@Gauravvvv Thanks for the work. Applied patch #3 cleanly on Drupal version 10.1.x. Please see attached screenshot.
- Status changed to Needs review
over 1 year ago 6:03am 28 March 2023 - Status changed to RTBC
over 1 year ago 1:58pm 30 March 2023 The last submitted patch, 3: 3337909-3.patch, failed testing. View results โ
- Status changed to RTBC
4 months ago 3:41am 30 April 2024 - Status changed to Needs work
4 months ago 9:26am 30 April 2024 - ๐ฌ๐งUnited Kingdom catch
This needs a conversion to an MR now, shame it's been stuck on random test failures for so long.
- ๐ฎ๐ณIndia Binoli Lalani Gujarat
Binoli Lalani โ made their first commit to this issueโs fork.
- Merge request !7848Issue #3337909 by Gauravvvv: Claro: Remove svg from indentation.html.twig file โ (Open) created by Binoli Lalani
- Status changed to Needs review
4 months ago 2:21pm 30 April 2024 - ๐ฎ๐ณIndia Binoli Lalani Gujarat
Hello,
I have converted the patch into MR. Please review.
Thank you!
- Status changed to RTBC
4 months ago 2:55pm 30 April 2024 - Status changed to Needs review
4 months ago 7:52am 1 May 2024 - ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
I'm not sure about this change. I think the reason this is inlined is due to core/themes/claro/images/icons/002e9a/plus.svg core/themes/claro/images/icons/545560/plus.svg etc... existing. If you need to have a plus icon in Claro then, I think, you should be using those. The svg in indentation.html.twig is for a very specific use-case - hence the inlining.
However, maybe the Claro maintainers will think differently, will ping them. However, if I'm right then perhaps this issue should be about adding a comment to Claro's indentation.html.twig about why the svg is inlined.
- Status changed to Postponed: needs info
4 months ago 1:49pm 1 May 2024 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
@alexpott is correct, this is a very specialized use case. It defaults to being a blank square, and there are several hidden svgs that use tabbledrag specific functionality to become visible on mousedown to display lines that visually connect related drag items. (so very coupled to tabledrag JS)
Keeping it inline makes it easier to maintain and allows for improvements to the Tabledrag UI without having to worry about BC with other uses of the image. I don't think it is worth moving it to a separate image unless there are specific use cases to justify it. The reported use cases seem more hypothetical.
If there is something more specific that seems best addressed by un-inlining the SVG, comment or update the IS with it an reopen, but as reported this seems like something best left alone.
- ๐ฎ๐ณIndia Gauravvv Delhi, India
Thank you @bnjmnm, for your response.
In the Stable theme, we're using space for indentation in
core/themes/stable9/templates/admin/indentation.html.twig
. We have different behaviors within the core themes. Can we make them similar for both? We can make changes to Claro or Stable. - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
In the Stable theme, we're using space for indentation in core/themes/stable9/templates/admin/indentation.html.twig. We have different behaviors within the core themes. Can we make them similar for both? We can make changes to Claro or Stable.
This is unlikely
The Stable theme, as the name suggests, exists to provide a stable set of markup and styling that will not make any changes that could potentially result in regressions (sometimes referred to as the "stable fence"). Updating Stable to work like Claro would be counter to this.
Claro is arguably a better implementation and would not want to roll it back to a pre-2019 implementation for the sake of making the approach similar in two themes - particularly since Stable's purpose is to provide a minimal frozen-in-amber theme intended for use as a base theme, while Claro is built to be an admin theme that accommodates innovations across updates.
Simlar to my stance in #25, if there's a non-theoretical real-world use case that this might address, making the change is a conversation worth having. If the desire is to have consistent approaches across themes without that real-world use case, it would not get FEFM signoff from me.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
f the desire is to have consistent approaches across themes without that real-world use case, it would not get FEFM signoff from me.
+1
Themes can have different approaches to CSS / HTML and design - that's kinda the point.