You can probably add XB to the list too :)
We're talking about adding something optional for which the only existing/current need is accordion/accordion_item. From there we assume things will get more complicated and try to come up with a solution, that doesn't sound like a plan that will address our users needs. In that situation where the needs are not clearly defined, contrib will implement it, battle test/refine the requirements and we get it in core once we know more about how it's used.
On the technical side of things It's totally possible for XB to add an extra yml file to SDCs that it will use (and maybe other modules can end up using the file too), core is not blocking anything except for this annoying restriction with finding SDC yaml files: š Make DirectoryWithMetadataDiscovery generic and reusable Active . As wim said in that very same issue: "In Drupal core, we generally only introduce an abstraction once there's >=3 uses for it." From what we talked about here, we're not there yet.
To me this issue should be postponed on XB implementing this, finding what works and what doesn't and we get the parts that are actually needed and helpful then.
SDC provides just that :)
Agreed
if we had the before/after thing for library this would be a 2 lines diff :)
always useful to harden the code, small code improvement needed
Can I get someone else to replicate the issue by following andyg5000 steps from #53, different subdomain, and with a vimeo or youtube video. And add the steps to reproduce to the issue summary update. Thanks! Not sure if it can be considered novice with the different subdomain setup, might get lucky :)
The point in #37 is also to consider, we'd add this to all oembed without distinction. I don't have an opinion if it's good or bad, might want to ask folks that use oembed a lot.
small code improvement needed
the wording need a little bit more work, let's try to get a native, documentation-inclined person to help out
Changing title and component because it's about the UI, not about the capabilities of SDC themselves
MR has the code for setattribute, need to do that for add_class too I guess?
updating credits
WordPress core defines about 90+ components that are reused and customized though classes and some custom API. They have more than a 100 fully "block themes" that are build on those (few themes define their own components, they mostly reuse the core ones to build pages) and since they consider their "hybrid themes" as block theme they're announcing more than a thousand block themes available, all built on those core components. It works for them and we don't have a radically different needs (XB has a lot in commun with gutenberg: problem space, technologies) so it'd work for us.
Closer to home, we have the navigation module that provides a "badge" SDC, no reason it couldn't be a global SDC that core and contrib can use. The goal is for core to move to SDC as a step to simplify the render API, see #2702061-106: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) ā .
It's not about a header from this and a footer from that, it's lower level. Like a card component, buttons it's always mostly the same. So yes, reusable SDC is definitely something that is necessary.
For reference there is already an issue for some of this, see related.
@pdureau, please go ahead with updating the documentation per specified in the issue summary, we can always refine later
I'm missing some infos somewhere because I do not know how we went from "restrict what's in a slot", to "let's add simple suggestions for the UI" to a whole new YAML condition API. I think I missed some IRL discussions somewhere. The terminology is confusing me too, at first I was wondering if the comment was on the right issue :p
I like the "policies" config idea.
I do not understand why the slot restrictions/conditions are in the SDC definition. @Wim talked about DTDs, huge fan, it's in a separate file that's specially made for that. Anything beyond "suggested" (and even that is arguably not at the right place) doesn't belong on the SDC layer for me, especially now that we agree this is a UI concern, not a technical (as in render-time validation) concern.
Additionally I'm thinking that adding all this at the slot level in SDC will make it very hard share SDCs across themes/modules. We'd need to have dependencies on other components to make it work. Not having this inter-components dependency mechanism is a feature, not a bug āØ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed .
Ended up here through
āØ
Allow SDCs to specify what components are allowed in its respective slots
Active
, I like $ref: module://experience_builder/image from #26.
We agreed on JSON-schema for props, let's follow-though on our decision and build on this as explained by @e0ipso and @pdureau (our 2 SDC maintainers). The DX concerns can be handled once things actually work, there are ways to address them as demonstrated by @effulgentsia in #33. We can have a whole layer of SDC generation that smooth out things for end users in contrib if it's not nice enough by default. The important thing here is that it works reliably in a non-drupal specific way
Sounds good, the complexity I'm worried about is if we try to enforce this at render time. Having a way to filter the elements in the UI is good I agree.
Yes that was not clear on my part, let me try to be clearer :)
SDC is not the right level because we would still want the design system rules to be applied when the component is overridden (I assume). And overriding a component just to change the rules for a slot in a yaml file is going to be a nightmare, now you own the whole component just to change a couple of YAML lines, how do you make sure it's updated? now the design system allows something else in that slot, how does it work with your overridden component? do you have one component by website if the rules are different? All these problems can go away if we work at a higher level, the declaration and enforcement of the restrictions should be at a higher level.
Declaring the restrictions at the theme level could work. IF at the theme level there is nothing that require changing a file on the filesystem to change the rules. We could have a file that declare the "default" restrictions but it's imported once or in an update hook and the file is not used at runtime. We want themes to be reusable so the rules should be easy to change by site builders without the need to deploy anything (designers will be happy about that too).
The way I see it at the moment is that restrictions should be tied to the website itself (maybe having a companion module for design system themes would be a good idea?) with the basic restrictions for reuse. If it's in a config entity we could even implement some of the rules though ECA, or let people alter them "easily" with ECA when necessary. Also we need to keep in mind all of this should be theme dependent, if you switch theme the restrictions should not apply anymore.
Not sure where the actual enforcement of the rules should happen (or if it should happen at all). I'm of the opinion that we need to let people do whatever if they access this from the code/render arrays directly, so enforcing would be only at the UI level in the various display builders interfaces. I don't think we should have code that check if this component is allowed in that slot at render time. That would mean only showing/hiding things in the display builder interface where/when necessary.
I'm just seeing a lot of complexity for a feature that will get in the way of the ambitious site builder (which is our focus, or marketers in the case of Drupal CMS) or, as Pierre said in #2, developers that need to implement a business requirement that doesn't follow design system rules but that was greenlit despite the Design team's opinion.
Small question left
I agree with pdureau that at the SDC level there shouldn't be any restriction on what can go in a slot. The HTML spec does not have any restrictions apart from the content of the slot being valid HTML. If we're starting to Drupalify the term "slot" by adding some implicit features, we might have a bad time later on.
The restrictions should be declared/implemented at a higher level. I like the idea of "suggested", because there is no code beside a new optional key in the yaml to support that. It's up to display builders to treat that as they want. XB could even take "suggested" key from yaml and make that "required" in the UI and adding the validation code there, nothing holding that up.
Yes I don't think any core theme uses this file
Thanks small nit on the test documentation
Paths needs to be updated after css file move
couple of things
nod_ ā made their first commit to this issueās fork.
updating credits
Committed 228328d and pushed to 11.x. Thanks!
Making sure this doesn't get forgotten somewhere
Committed and pushed 8610d622f11 to 11.x and 43c52bdf4a4 to 10.5.x and 750f5decb21 to 11.1.x and d5035b8d3c0 to 10.4.x. Thanks!
griffynh ā credited nod_ ā .
Committed 49373ed and pushed to 11.x. Thanks!
I think that's worth a release note
Committed 784effb and pushed to 11.x. Thanks!
That makes sense, will be easier than ignoring one file at a time
per thread on slack we're not backporting the top bar work to 10.x, closing this one
per thread on slack we're not backporting the top bar work to 10.x, closing this one
let's worry about 11.x first and we'll handle the backports once it's RTBC/committed
nod_ ā changed the visibility of the branch 2741877-nested-modals-10.1 to hidden.
nod_ ā changed the visibility of the branch 2741877-nested-modals-10.3 to hidden.
nod_ ā changed the visibility of the branch 2741877-nested-modals-10.2 to hidden.
all good, thanks
There is a phpcs failure, needs to be fixed before I can commit
Committed c206e83 and pushed to 11.x. Thanks!
appears random š Remove cache tag checksum assertions from performance tests Active
test failure that seems legit
Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testStandardPerformance
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
],
'CacheSetCount' => 45,
'CacheDeleteCount' => 0,
- 'CacheTagChecksumCount' => 38,
- 'CacheTagIsValidCount' => 43,
+ 'CacheTagChecksumCount' => 37,
+ 'CacheTagIsValidCount' => 42,
'CacheTagInvalidationCount' => 0,
'CacheTagLookupQueryCount' => 21,
'CacheTagGroupedLookups' => Array &2 [
core/tests/Drupal/Tests/PerformanceTestTrait.php:679
core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:175
core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php:57
I didn't know this tag, would have been useful to me a while back, nice :)
Committed 086f524 and pushed to 11.x. Thanks!
Committed f3d4fc3 and pushed to 11.x. Thanks!
small suggestion
nevermind, git is smart enough
sorry another conflict in dictionary.txt, once rebased ping me so I can commit quickly hopefully this will be the last rebase
merge conflict after š Navigation leverage icon core API Needs work
Committed and pushed 667827baf8f to 11.x and b6569266a24 to 11.1.x. Thanks!