Currently, Layouts can provide their own icon image which can be used in user interfaces. These icons could be in any size, color, dimensions, or style. As a result, user interfaces suffer as consistency is lost between different Layouts. The best example of this are the bright-pink icons provided by Radix, which look nothing like the default Panels or Display Suite Layouts (this isn't Radix's fault, we can't expect everyone to have the same design opinions).
The provided patch is a proof of concept for dynamically generated icons, which can not only help with consistency but could also be used in user interfaces to directly interact with regions. This also reduces developer strain as filling out config is far easier than creating an icon (which is a skill not everyone has).
Instead of making something fully featured, I wanted to put this forward and see if anyone sees value in what I've already done. Simply apply the provided patch, enable the layout_plugin_example module, and visit the path /admin/layout_plugin_example/layout_map/layout_example_stacked.
What you see on that page is the result of a new configuration value, "icon_map", which is a two-dimensional array mapping region names to their visual placement on a page. Widths of regions are determined dynamically based on the number of columns per row in the array. You can also hover over any region to see a tooltip containing the machine name of the region. See layout_plugin_example.layouts.yml for the exact structure used.
Thoughts?
Comment | File | Size | Author |
---|---|---|---|
#116 | 2660124-layout-116.patch | 23.94 KB | tim.plunkett |
#116 | 2660124-layout-116-interdiff.txt | 4.45 KB | tim.plunkett |
#106 | icons_in_tray.png | 20.35 KB | xjm |
#105 | layout-icons-2.gif | 1.33 MB | yoroy |
#103 | 2660124-layout-103.patch | 23.96 KB | tim.plunkett |
Comments
Comment #2
samuel.mortensonMinor code cleanup that I missed last night.
Comment #3
samuel.mortensonAdded a tl;dr screenshot of this working on a temporary stacked layout.
Comment #4
dsnopekFor the record, this is really cool! And making those layout icons has always been super annoying. :-) I'd love to get this in eventually.
As discussed on IRC, the thing that worries me is this slowing down our attempt to get layout_plugin into core. Maybe we can do this in a contrib module for the time being and merge upstream later (whether upstream means layout_plugin in contrib or core)? Or if we completely miss the core 8.1.0 window, then we'll have 6 months before we can try again for 8.2.0, and then I'd be less wary of merging this in..
Comment #5
Gábor HojtsyThis looks super cool and indeed the 8.1 release went by... Not 100% sure this needs to be in the initial solution but this is an important usability problem we struggled in Spark years ago when doing dynamic layout editor. Its great you can dynamically edit your layout but then there was no dynamic icon. So super-cool once again :)
Comment #6
webchickJust wanted to chime in and say this is fricking awesome.
Comment #7
phenaproxima+1 for this feature. It is radical, tubular, and righteous. Love it.
Comment #8
johnkennedy CreditAttribution: johnkennedy commented+1
Comment #9
tim.plunkettStealing this now that Layout is in core.
The global function and render() calls were bugging me, so I made it a service? Idk.
Also wrote some tests.
Since we don't have examples, didn't port that part.
Also have yet to add it to the layout plugin at all.
Comment #10
tim.plunkettComment #12
tim.plunkett...
Comment #13
samuel.mortensonAwesome! It's worth noting that the patch from #12 includes the field_layout patch from #2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts. I'll take some time to review my logic for rendering the icon since I haven't worked on this in a year and want to make sure there's nothing (major) missing.
Off the bat I would recommend that core only uses dynamically generated icons if this goes in, to set an example for contrib developers. In addition to all the benefits of being configurable in YML and styled via the API, these icons also have hover tooltips on regions so you actually know what goes where.
Comment #14
tstoecklerPlayed around with this a bit. Because of #2544318: Remove #type => html_tag usages I don't think the current render mechanism is going to fly at least in the long run. I think we should add proper theme functions/render elements for generating SVGs. Cooked up an initial patch at #2694535-5: Support rect property and nested render arrays in html_tag for dynamic SVGs. Attached patch shows how that could work. It also adds all layout icons to the "Manage display" of entities with the field_layout module on for testing purposes. (It is meant to be applied on top of #12)
Comment #15
tim.plunkettRolling this all together a bit more. The do-not-test branch shows without the initial field_layout patch, but contains #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs
Comment #17
tim.plunkettComment #18
samuel.mortensonShould we switch this logic to prefer the dynamically generated icon?
In my opinion the icon path is a fallback in case there isn't a map, although you may have done this to prevent confusion with existing layout_plugin users.
Otherwise this patch is looking good! Thank you @timplunkett for all the work so far. I also added some review to #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs to move that issue along, as this depends on it.
Comment #20
dead_armAt DrupalCon Baltimore @pixelite and I were discussing that the layout preview icons that Panopoly provides are more legible (black rectangles without borders) than the layout preview icons that Panels provides. Our suggestion is that the generated svg layout icons should be solid color rectangles instead of rectangles with borders.
Standard Panels layout preview:
Panopoly layout preview:
Comment #21
tim.plunkettThis is a straight reroll of the last patch. It does not take into account changes made in the other issue, or the review in #20!
Comment #23
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedCame across this while working on #2786541: Add page listing the available layouts on the site, and this is awesome.
Just a small request:
It'd be great if we can get the
alt
attribute on the icon as well:'#alt' => $this->getLabel()
Comment #24
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe: #20 that looks better, but as it is it would not work if the theme had a dark background... unless we set the background of the layout selection to white, or add a white background to each icon itself.
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedJust a heads up that #2694535: Support rect property and nested render arrays in html_tag for dynamic SVGs is RTBC, with a new approach which allows nesting html_tag elements, see CR https://www.drupal.org/node/2887146
Comment #26
samuel.mortensonThis should fix tests, I updated \Drupal\Core\Layout\IconGenerator to support the recent html_tag changes and things are looking good! #20/#24 are not addressed.
Two things we need to think about:
Comment #27
WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commented@samuel.mortenson,
Great work on this. While testing #26 locally using the field_layout module, I noticed a strange issue. Every time I switch back and forth between different layouts, it seems to nest the layout preview in another div.
So initially you have something like this:
But then after switching several times you get a bunch of nested divs like this:
I'm not entirely sure if that's related to this code specifically or a sign of a separate underlying issue with the field_layout module, but thought I'd point it out here. Since I'm not entirely sure the culprit, I won't change the status of this issue just yet, but wanted to point that out.
Comment #28
tim.plunkettThat's #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs
Comment #29
WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commented@tim.plunkett ahh okay great. Disregard my comment then.
Comment #31
tim.plunkettThe Layout system is no longer experimental! Yay.
Since this is a service, it should have an interface.
There should be a layout in layout_test that has a PNG. We shouldn't add one to just one core layout.
Comment #32
tim.plunkettFor better usage in different situations, I think getIcon() should have a $width and $height parameter, which can be passed to the #theme=>image array and the generate call
Comment #33
tim.plunkettI made the adjustments of #31 and #32, and also finished defining the config for each layout, and added some more docs.
Comment #35
tim.plunkettAccidentally changed the default values when I was testing it out. Set them back.
Comment #36
tim.plunkettgetIconMap should be made a real method, and $icon_map should be added as \Drupal\Core\Layout\Annotation\Layout::$icon_map
Comment #37
tim.plunkettFinally got back to this.
Comment #38
yoroy CreditAttribution: yoroy at Roy Scholten commentedSuper cool to see this back alive!
Keeping it at the same size and changing the colors only already makes it visually less intrusive:
Comment #39
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRE: #38.3 - While I agree that removing the stroke makes it much easier on the eyes, if the theme has the same background color of gray, you would not be able see the gutters...
Comment #40
SKAUGHT#39
simply enough: add a 10px white border (padding) around the entire image.
Comment #41
yoroy CreditAttribution: yoroy at Roy Scholten commentedGood point @Manuel Garcia. Discussed this with @timplunkett in IRC:
- Size is based on the icons being used in settings tray, like so: https://www.drupal.org/files/issues/layout-builder-add-section.png
- Colors can't be overridden
I experimented a bit more. Screenshots include the widhts, heights, colors I changed through the inspector. You can see that with these values the whole of the 250*300 canvas is used for the drawing, the blue area indicates the whole of the svg box:
using 2px strokes:
using 1px strokes (This still needs a x=1 and y=1 starting position or else we see a blurry attempt at 0,5px rendering)
A 1px stroke looks better. The 2px stroke interferes with the 2px(?) gutter. Having the stroke be less wide than the gutter is easier on the eye.
I used #333 for the stroke color (same as for body text) and #ccc for the fill.
A more Seven compatible palette: fill #f5f5f2, stroke #666 (because double #333 and well, evil :)
The same drawing on the dark settings tray background:
Comment #42
tim.plunkettWorking on this.
Comment #43
tim.plunkettRedid the generation logic to account for the stroke and padding. SVGs draw their stroke centered over the line. For a 4px stroke, you'd need to position the top left at 2,2 in order for it to be visible.
This uses the colors specified by @yoroy.
Additionally, I removed all of the default values for color and widths from the service.
Since they cannot be overridden, I believe they should always be specified by the calling code that is putting it into the UI.
This also prevents us from putting Seven theming into \Drupal\Core subsystems.
Comment #44
tedbowUPDATE: forgot to say how awesome this feature is!!! 🙌
Is the only place someone could look in the code to determine what should go an icon_map? Or or at least to visually what an icon would look like next to the map values?
If so it seems like it would be good idea to show another example with region spanning multiple rows.
Why is a public method on the interface? From what I can tell it is only called from
\Drupal\Core\Layout\IconGenerator::generateSvgFromIconMap
Do well really want to expose this to our API if we don't have to?Comment #45
tim.plunkettGreat feedback on both points!
Fixed both.
Comment #46
neclimdulLooks really awesome. Truly the dream for layout icons.
I had some concerns about the find-ability of of the icon_map format documentation. I think its probably fine with the @see but it does bury it a bit. I'm told its because we want to provide logic that can be used outside the core layout plugin and that makes sense.
Additionally render arrays... gross. I guess this sort of kills #2544318: Remove #type => html_tag usages(noted in #14) but without a SVG library or something much more complicated that seems like the only solution so carry on.
Comment #47
tedbowCould probably use the same @see to \Drupal\Core\Layout\IconGeneratorInterface::generateSvgFromIconMap
So it would show up here https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Layout%21...
Otherwise very close.
Comment #48
dawehnerI just came by to say how amazing this feature is!
Here are just nitpicks
Given the rest of the function this if could be avoided. The rest doesn't do anything
Nitpick: Let's typehint against the interface
Comment #49
tim.plunkettFixing all the code parts.
Docs and CR still needed.
Comment #51
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAny chance we can get the
alt
attribute as mentioned on #23? It would be great for accessibility.Comment #52
tim.plunkettSVG elements don't support alt, but a top-level
<title>
tagAdded support for that, as well as adding #alt for the image.
Also the check I removed in #49 *was* needed, but fixed it a different way.
Comment #53
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedBrilliant, thanks @tim.plunkett!
Comment #54
samuel.mortensonCreated change notice: Layout icons can be dynamically generated
We can add more information, but I wanted to keep things simple as the documentation will likely go into more depth.
Comment #55
tstoecklerSo I stumbled a bit on the number of arguments here, which makes usage of this function a bit confusing IMO, and then I noticed that with
$regions
we already prepare a special data structure to assemble the rectangles from. So I thought whether it would make sense to add the fill, stroke and stroke width to the$regions
structure as well. That would have the upside of requiring fewer arguments to this function and also allowing a bit more flexibility if someone needs it. The downside is that it would make$regions
a fair bit more magical. So I guess it's arguable whether it's good or bad, but I thought I'd put it out there.This sentence is not quite proper English.
Am I missing something or are the
getOffset()
calls not identical between the if and the else branch? Would be nice to consolidate. Especially the 'x' and the 'y' part in the if-branch look inconsistent even though it seems they should not need to.So it seems these are only used for the internal calculation but they are never cleaned up afterwards. They don't really hurt, but since nowadays people are just as likely to just look at the resulting object in a debugger rather than (or in addition to) reading its docs, I think it would be nice to make it a tad less confusing to those people, given that that this is just a magic array.
Comment #56
tim.plunkett#55 Thanks for the review!
1) I just looked into making $regions more magic, and also into making an $options array containing the more optional parameters.
Except that both made the code much harder to understand.
But it made me look into what happens when you pass NULL for any of those.
Thankfully it prevents the attribute from printing at all (which is good), and that SVGs natively have default values for these.
So as a compromise for the long list of parameters, we can make the truly optional ones optional. I think this is an improvement.
2) Reworded
3) Well spotted! I guess I didn't clean up after refactoring this.
4) I'm eyerolling super hard at the idea of managing our internal data differently in defense of people violating the API. But it's an easy change to make, and a fun usage of array_walk :)
Comment #57
zaporylieNitpic: @var takes data type specification only. "optional" should be moved to the description (2 lines above) or removed. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a... and #2909370: Fix 'Drupal.Commenting.VariableComment.IncorrectVarType' coding standard
Comment #58
tstoecklerRe #56: Thanks, the improvements look great. Regarding 1: Thanks for thinking about it, I absolutely trust your judgement that it would make the code less readable. And thanks for 4., I will applaud your patience with me every time I see one of those thingies in the Debugger ;-)
Comment #59
tim.plunkett@zaporylie, this was added to our standard specifically for documenting annotation classes, fixing that standard (and all annotation classes) is out of scope here.
@tstoeckler ❤️
Comment #60
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedBrilliant, thanks all!
I had a look at the CR, it looks good to me. So I suppose the only thing left to do here would be updating the documentation.
Comment #61
DyanneNovaHere's a draft for the documentation changes:
[ ... ]
( I removed current code tags to keep formatting readable here, but we should keep them in the updates.)
Comment #63
tim.plunkettLooks great, thanks @DyanneNova!
Leaving the tag, since we still have to make the changes once it is committed.
Comment #64
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWas this RTBC'd before by someone who didn't work so much on the patch?
Comment #65
tim.plunkettIt was not! My mistake, I thought it had gone RTBC->NW in #60. Thanks @amateescu :)
Comment #66
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedLooks to me like all code reviews have been addressed, we have a CR and Documentation edits ready for when this is committed. RTBC++
Only one thing I see that could be improved perhaps.
@see would be better =)
Comment #67
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedMy last nitpick should be fixable on commit :)
Comment #68
xjmThis is interesting!
Miscellaneous questions and small things to fix, mostly docs clarifications:
I was initially confused by the difference between
generateSvgFromIconMap()
and this, until I noticed that this is protected and not on the interface. Would this method be better names something likegenerateSvgRenderArray()
or something? Because it doesn't actually generate an SVG per se.Discussed with @tim.plunkett; this is a
mixed[][]
.See which bit of `generateSvgFromIconMap()`? I'd have assumed its return value but it looks like maybe it actually means the `$icon_map` parameter? Let's say that explicitly?
Also I guess they are `string[][]`?
Nit (here and elsewhere): "two-dimensional".
The suggestion above can be addressed by additionally appending the
@see
at the end of the docblock I guess; I do think we should refer to it here in the parameter docs for clarity (where we can't have an@see
).What if I wanted 0 padding, like a stained glass window? (Or
border-collapse: collapse;
for tables.) I would think 0 is a fine value then?By the same note, could it be a fraction value? (Applies to the others as well I guess.) I think it's fine to choose to constrain it to integer pixel counts by choice; just wanted to raise the question.
Er wha?
Wouldn't this be twice the stroke width (a border on either side)?
...Never mind, I see above that only half the stroke is outside the region. Having inline comments here could be helpful in case we have to update the math for some reason in the future.
Are the region names strings here? If so could we single-quote them for clarity in the example, and declare the parameter as
string[][]
?Above in #20, someone proposes making the layouts solid rectangles for easier visibility. Per @tim.plunkett, the colors in the current patch have been checked for contrast ratios and were recommended by @yoroy (as a Seven-compatible palette?)
Where do I specify the background color? Or would I just do that in CSS?
Do we need a followup issue to add CSS to themes for this (postponed on it being stable), or do we want it to always be specified in the API?
Comment #69
tim.plunkett#68
1) Agreed, the naming here is off. I went to rename the methods away from "generate", but stopped since it is also the class, interface, and service ID. Let's discuss that naming before changing it all.
2) Fixed! It is technically
(int|float)[][]
but that is not valid doxygen :)3) Fixed
4) There's no reason to say this, even negative values are valid (and useful, in the stained glass example).
Fixed the docs and added test coverage.
Technically there is nothing preventing a fractional value, and it would render just fine.
5) While attempting to improve these docs, realized we shouldn't ever need to store the original values. Hopefully this is more understandable
6) Added docs
7) This was copied from the YAML, where quotes are not included for machine names. Adding them here though.
8) There is currently no provision for background color. Yes you could do it in CSS, or we could add an additional parameter.
Themes can override the colors provided with CSS, but I agree we should ship with reasonable defaults.
@TODO Decide on naming (generate, build, etc)
Comment #70
tim.plunkettDiscussed the naming in the #layouts Slack channel with @EclipseGc and @samuel.mortenson.
Decided that just removing "Svg" from the method names made it clearer.
Yes, it generates a render array, which is documented. And now the method names don't imply something different.
That covers the last of the feedback in #68
Comment #71
larowlanReviewed this, my only question is about the precision of the fractional numbers in the test-case.
Code looks good, is nicely isolated and well tested.
Is there a risk that these might result in a fragile test? i.e floats are always painful w.r.t precision
Comment #72
EclipseGc CreditAttribution: EclipseGc at Acquia commentedLooks good to me!
Eclipse
Comment #73
markhalliwellMinor, but important documentation nit: each "Defaults to" does not match the actual default values.
Comment #74
phenaproximaAddressed #73.
Comment #75
tim.plunkettWell spotted @markcarver, got out of sync during the feedback cycle.
Thanks for fixing @phenaproxima!
Comment #76
xjmThe naming changes still don't really address differentiating
generateFromIconMap()
fromgenerate()
. They don't return the same thing, but they are named as if they do. Can we discuss the naming a little more?Also not sure the bit about background color got addressed. From @tim.plunkett in #69:
Thanks!
Comment #77
tim.plunkettBut they do return the same thing? The only difference is the parameters
One literally returns the other after translating the icon_map into the x/y/height/width array
Comment #78
xjmDiscussed with @tim.plunkett. My suggestion is that the method currently named
generateFromIconMap()
(which is the only public API) just be calledgenerate()
. And then the internal helper be called something likebuildRenderArray()
, since in this case you don't actually have a choice between two different generation methods. (Tim explained that both methods used to be public, so I can see how we ended up with the current pattern.)We also discussed whether a parameter for the background color should be added, or whether the stroke and fill colors should be specified in the PHP API at all. @lauriii said he did not think that the colors should be specified via the API. So let's remove those colors as parameters and figure out how to best supply default styling for them in the theme layer.
Comment #79
tim.plunkett1395f35749 Rename methods.
e98e8348e2 Remove support for specifying colors.
8a4c1163b5 Add back colors via CSS.
Comment #81
tim.plunkettForgot to update the test for the attribute additions.
Comment #82
DyanneNovaI think it would be helpful to include a class on the svg for the layout label, so, for example, in addition to layout-icon we would also have layout-icon--two-column on the two column layout icon. That will make the region classes more useful and give layout themers an easy way to categorically color layouts for site builders.
Other than that, the CSS looks good to me!
Comment #83
tim.plunkettGood idea! Fixed.
As I made this change, I realized why this class has been bugging me: The parameters to generate() are getting out of hand.
Every time we want to pass slightly more data to the generator, the entire method signature has to change.
So I rewrote the whole thing using a form of the Builder pattern.
Take this usage from LayoutDefinition:
All that is strictly required is the icon map.
The rest are overrides or enhancements.
Comment #85
tim.plunkettMade a mistake with the reroll for #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
Adapted the location of the CSS files to fit with the new structure.
Comment #86
EclipseGc CreditAttribution: EclipseGc at Acquia commented$stroke_width isn't default null anymore. Is this an error?
Otherwise looking good
Eclipse
Comment #87
tim.plunkettI should have mentioned that, sorry. I did this purposefully. This should have been done when the stroke color moved to CSS.
Comment #88
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk then, let's commit this, it looks good!
Eclipse
Comment #89
xjmHa nice catch. Missed that.
Probably we should have a followup to add these to Classy once the module is stable?
Comment #90
xjmDisregard #89; I apparently somehow opened an older patch. And of course this is being added as a stable library so it doesn't need a followup; it just goes in this patch. :)
Comment #91
xjmI like the switch to using class members for the width, height, etc.; I had thought about proposing that when I reviewed it previously but eventually didn't mention it. But I like the change. I think we need to go a little further with it though.
Discussed with @tim.plunkett. I was wondering if
buildRenderArray()
should either be static (and take more things as parameters) or be purely "self-aware" (cr. @tim.plunkett) and just use the members rather than taking them as parameters. And then I started thinking about state and Tim remembered that the icon generator is still a service. So it should have an icon generator factory service (icon factory service?) that's separate from the icon things that are things. (Y'all with the CS degrees supply the correct terminology here.)So NW for that; Tim has a refactor that he's probably already coding to address that. :P
Comment #92
xjmAlso, let's test this in an RTL language to see if it supports RTL currently.
Comment #93
tim.plunkettNeither
<svg>
nor<rect>
supportdirection
, only the text content within them.Any file-based icons would similarly not respect RTL.
Furthermore, our layouts themselves apparently do not respect RTL!
Opened #2919372: Layouts do not respect RTL as a follow-up.
This switches to a factory in order for the service to remain stateless.
Comment #94
tim.plunkettThat was just the interdiff. Here's the patch too :)
Comment #95
EclipseGc CreditAttribution: EclipseGc at Acquia commentedIconGeneratorFactoryInterface?
SvgIconGenerator?? seems weird to not include that word. If not, that's cool too.
SvgIconGeneratorFactory
I'm sure I'll say this on the services.yml but the service id feels like it should be "layout.icon_generator_factory" now, and even as I read the interface above, I was thinking "getGenerator()" which would make this line pretty clear. I'm being pedantic here probably but if it resonates for you, then consider it.
Yeah, as I mentioned, feels like this should be "layout.icon_generator_factory"
Code looks pretty good otherwise, my complaints are all "hard problem" territory. I think I'd like to see "generator" in the interface and class names as appropriate. If you feel like I made a point worth considering on the service, then do that too, but I'm not married to it.
Eclipse
Comment #96
tim.plunkettAll the way back in #68, @xjm pointed out that we don't "generate" anything.
@EclipseGc also said in chat something along the lines of "build is shorthand for 'i want a render array'".
So this once again switches the names.
Additionally, @EclipseGc pointed out there was a feature in Symfony that ensured we wouldn't get the same instance of a service. This lets us skip the factory part.
Comment #97
EclipseGc CreditAttribution: EclipseGc at Acquia commented\Drupal\Core\Layout\Icon\IconBuilderInterface
Otherwise this looks great.
Eclipse
Comment #98
tim.plunkettWell spotted!
Comment #99
EclipseGc CreditAttribution: EclipseGc at Acquia commentedAny reason to not be more specific here and say something like "Builds render arrays representing SVG layout icons" or something like that? We're not actually building icons from this thing.
These are all suppose to be integers and the rest of the code completely depends on that. I think it's probably worth doing a check to make sure that we got what we were expecting since we don't have scalar type hints in PHP 5.5.9.
Super happy this worked.
This looks really really close.
Eclipse
Comment #100
tim.plunkettI'm going to push back on both of these points.
1)
\Drupal\Core\Layout\Icon\IconBuilderInterface::build()
is the one that cares about render arrays. But there could eventually be non-render array public methods. And \Drupal\Core\Layout\Icon\SvgIconBuilder is just "whatever the interface says, but with SVGs", hence the shortened comment2) We don't actually enforce int, it could also be a float in theory?
If assert() weren't currently so fraught with PHP 5/7 issues, I'd consider using that.
But there's nowhere else we do this sort of handholding.
Comment #101
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, I'm convinced. This patch looks super solid and reads pretty nicely. I say we get this done.
RTBC pending tests
Eclipse
Comment #102
yoroy CreditAttribution: yoroy at Roy Scholten commentedWorks like a charm, really nice. But why the full-on super dark fills? It's not the best looking default and in #38/#39 we discussed that a border-less design will make the icons invisible on a background of the same color.
Comment #103
tim.plunkettI made a mistake in my reroll in #85 which caused #102, fixed that.
Also per discussion with @xjm, tried to reduce the number of places we hardcode these numbers.
Couldn't remove them all, as we need a height/width for the file-based portion of LayoutDefinition::getIcon()
Comment #104
xjmThanks @tim.plunkett; that set of defaults seems fine to me.
Comment #105
yoroy CreditAttribution: yoroy at Roy Scholten commentedMuch better!
Comment #106
xjmI manually tested it with Field Layout and it works great, as in the screenshot in #105.
I also used #2905922: Implementation issue for Layout Builder to test the icons in the Settings Tray. They resize nicely with the overridden sizes from that patch. The contrast ratio in the Settings Tray is a little intense:
But since the layout icons aren't exposed in the Settings Tray in this issue or in HEAD, I don't think that needs to block anything here. (It might even be okay anyway.)
Comment #107
xjmI checked with @yoroy and he said the colors in #106 are fine, so no blocker there.
Comment #108
xjmI like where this has ended up; probably will commit this later today.
This combo of names makes a lot more sense, thanks! And it's nice that only one thing is required to create it.
I would have expected this to be injected, but @tim.plunkett explained that it's not injectable, and pointed me at the
entity_type
service which is used in the same way. It still seems a little weird to me, but not worth blocking this on.Comment #109
xjmAdding #2919372: Layouts do not respect RTL to the related issues, which I think blocks both Field Layout and Layout Builder from being stable. But since this and Layout Discovery only provide APIs that are not yet exposed in stable code, it's not blocking here.
Comment #110
larowlanDid anyone answer my question in #71 about the level of precision here? Is it needed? Could it lead to fragility because of floating point arithmetic? Pity we can't use calc
Comment #111
tim.plunkettI could pick more round numbers for the test maybe? But browsers support the subpixel arithmetic, and I'm not sure of a better way to flub the math to get integers always.
Comment #112
larowlanAccording to https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/width width/height can be specified in % - would that help - or would we still end up with decimals?
Comment #113
tim.plunkettWe'd still end up with
33.333333% 33.333333% 33.333333%
and the likeComment #114
larowlanOkay - thanks - was worth asking.
So the only other option is some maths to make it integers, but that might yield issues. We could explore that in a follow up, but its probably not worth the effort.
Thanks again
Comment #115
markhalliwellThe level of precision does seem unnecessarily verbose. I think, at most, 3 levels would be more than sufficient.
There is the possibility that these tests may fail in the future if this current level of precision ever changes depending on the software/architecture, however that seems highly unlikely... at least for a while anyway.
On a side note, I found this relic of an issue and think this issue provides a perfect use case for adding it to core, but I don't think it's a blocker for this issue.
For now, if anyone cares enough to do this, simply adding some @todos for the relevant methods/tests that reference said issue would likely be enough.
Comment #116
tim.plunkettSwitched the dimensions around so that we still test that floats are supported, but by using halves instead of thirds.
Leaving at RTBC.
Comment #117
xjmI don't think #115 is really relevant because this isn't a field; it's an HTML attribute. I do think we should test float values to ensure they work, because that's important to make the icons render correctly and it's going to be the case more often than not.
#116 seems a good compromise to avoid any risk of testing implementation details of the testbot while still testing the fractional values. Thanks @tim.plunkett.
I think this is ready, but waiting for tests.
Comment #118
xjmSaving issue credit.
Comment #120
xjmAlright, I read through the whole patch one more time, including executing the code and the test scenarios in my head with mental math. :P I also manually tested with both field_layout and the Layout Builder patch (but not both at the same time since field_layout breaks the Layout Builder).
Committed and pushed to 8.5.x, and published the change record. Thanks!
Comment #121
DamienMcKennaWoohoo, thanks everyone!
Comment #122
ipwa CreditAttribution: ipwa commentedThis is so amazing, thanks guys!
Comment #124
tim.plunkett