Test failures in MR are because the tests are checking for NULL rather than an empty render array with only cache metadata.
Simplified and fixed some inaccuracies with the original code (not the previous patch), and updated to apply to 2.1.
Adding a code-only patch.
Test are passing now.
We have an interface change and I'm not 100% sure about one bit, but I'm going to mark this as needs review.
MR !14 takes rob230's approach in #9 and updates it to also work with the chained and core file resolvers.
However, I still think that it is strange / incorrect to count usages from both the entity usage and core resolvers as mentioned in #5 and #12. They each count the same usage, so it's basically always doubled. But since we're now removing self usage and we're only ever really concerned if usage is >0, maybe it doesn't matter?
I'm seeing that with the media_file_delete_entity_usage submodule enabled, the usage count is doubled when checking before deletion.
I see that in ChainedFileUsageResolver::getFileUsages(), the usage gets calculated once for the EntityUsageResolver, and then that gets added to the value from the CoreFileUsageResolver. Both resolvers see 1 usage, but they get added together to make 2.
public function getFileUsages(FileInterface $file): int {
$usage = 0;
foreach ($this->getSortedResolvers() as $resolver) {
assert($resolver instanceof FileUsageResolverInterface);
$usage += $resolver->getFileUsages($file);
}
return $usage;
}
Probably we need tests, and to test with multiple languages, but MR 35 is working for me with english synonyms.
I like option 1, but I don't understand the challenges required to implement. I don't think I like it if it adds a lot of complexity. And I worry about removing the option and defaulting to no sanitization even for fields we think are safe if it's already been out there sanitizing these fields.
RE: Option 2, hide the option from the UI - my personal feeling is that this just makes it more difficult and confusing to use this option, but doesn't really prevent it from being misused. I think folks will search for an answer and then just make the config edit because they saw it in some stack overflow answer without understanding it anyway. And maybe that is even worse with AI suggestions? I would personally feel better about showing the option with a warning, a situation where I can control the information the person gets before making the change. 🤷
But I think it's a close call and I'll support whatever path you choose.
I just want to note that while it defaults to view-content, there is an option in the show more pager configuration for a view to set what the container class should be. I suspect this is not very helpful because I'm seeing that there's no container in some themes rather than a container with a class other than view-content.
MR 18 updated to apply to the latest 2.x dev.
Attaching patch equivalent of MR 18 for use with composer.
sonfd → created an issue.
Needs review and credits.
This needs credits and review.
I really wanted to use the block's machine ID as the block ID, but this doesn't seem reasonably possible, see issue #2540088 💬 Retrieve unique block ID or machine name in build() method of BlockBase class Active .
Instead I took a page out of core's book and used the Crypt:randomBytesBase64() to generate a unique ID. (It's called from Html::getUniqueId() for Ajax responses.)
I am not familiar enough with this API to RTBC, but the updated delete logic looks good to me. And so does the new test logic.
But since deleting a checkpoint currently results in a broken state, should we also add an update hook to fix the broken state on existing sites?
sonfd → created an issue.
Thank you @chhavi.sharma -
I left some feedback in the MR. The biggest thing is that if we're not using the 'disclosure_button_label' setting anymore, we should remove it from the form. We need to keep the default value and leave existing values as-is to ensure that we don't break things for folks with an existing block and overridden template.
This needs more work.
- We'll need documentation of the new option
- We may want to preserve more cell formatting other than just
strongandem - We'll need tests
Also, the reason I got interested in this issue is because I wanted to preserve links in cell content, but I could not figure it out. :sigh: I suspect that might need work in the upstream PHP spreadsheet package.
This needs review and credits.
Thank you @smustgrave, I think I see how it could be done now. But I have run out of time in the short term, I'll try to get back to this to add the test of the upgrade path sooner rather than later.
I added an update hook that updates FieldBlocks to set show_when_empty to FALSE, but I'm not sure if there's more I need to do there. See my previous note about whether more is needed.
But I am not sure how to go about writing a test for the update hook.
Thank you, I understand now.
@smustgrave -
Sorry, but I want to make sure I'm understanding you correctly.
Currently the FieldBlock's default configuration has a value for the new config field that matches the FieldBlock's behavior pre-patch.
Are you saying that we need an update hook to update all FieldBlocks to set a value for the new config option and then a test to make sure update hook works properly? Or something else?
MR !12052 refactors original approach to use a "show block when field is empty" checkbox rather than a "hide for empty" checkbox. This MR also includes updates to tests to test the new option.
As a result, I'm going to mark as Needs review and remove the needs tests tag.
My instinct is to also hide MR !5111, but I'll leave that to a maintainer.
First, I just want to note that we should not be iterating on patches since we're using an MR. Any work and iteration should happen in the MR.
Second, I'd propose that the logic of this new field be flipped: instead of "Hide when empty" and defaulting to checked, use "Show when empty" and default to unchecked. I will add an MR with this alternate approach.
I have been thinking about this and I think we want:
- to break the main template into smaller templates
- folks to be able to override the smaller templates without necessarily needing to override the main template
I think the second goal is maybe more of a nice-to-have than a requirement. If the main template just includes a bunch of smaller templates, it should be easier to override it without it being a problem.
I know the point of Single Directory Components is that all of the css, js, etc are all in one directory, but I wonder if using them for the smaller pieces of the main disclosure menu template is the best option, even though I think we'd still keep the css and js all in one place, attached to the main template. Each sub-template could be an SDC, basically a more defined include, and would really just define a template. SDCs would allow us to easily define the smaller templates in a way that would allow any theme to override any one of those templates easily without modifying the main template. 🤷
Needs review and credits.
sonfd → created an issue. See original summary → .
Needs review and credits
Needs review and credits.
Needs review and credits.
I could not track down a d.o username for Akhil Babu
@mortona2k, I was just coming back to this after sleeping on this to say basically the same thing! Except I think I'd recommend the additional checkbox be "Expand items in the active trail" and we can default it to checked.
Actually, scratch pretty much everything I just said.
I'm seeing in MenuLinkTree::getCurrentRouteMenuTreeParameters(), we are explicitly expanding links in the active trail:
/**
* {@inheritdoc}
*/
public function getCurrentRouteMenuTreeParameters($menu_name) {
$active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
$parameters = new MenuTreeParameters();
$parameters->setActiveTrail($active_trail)
// We want links in the active trail to be expanded.
->addExpandedParents($active_trail)
// We marked the links in the active trail to be expanded, but we also
// want their descendants that have the "expanded" flag enabled to be
// expanded.
->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));
return $parameters;
}
If we're going to do something here, I think we have to do it there.
I tried to update the issue summary to be a little more clear. I do think this is a duplicate of ✨ Menu item checkbox "Show as expanded" not working properly Needs work , but I think the approach in this issue is the correct way to go so I'd recommend closing the other one.
To @aaronchristian's point, I think this patch is only complete with a change to the template file. Maybe I'm wrong, but I think we shouldn't even be building the subtrees of a menu item if it's not expanded. And if we do that, we won't need a change to a template file. I'll take a stab at that in an MR now.
I think the approach in Issue 3424786 → solves this problem in a more complete way.
I think the approach in the MR is more complete than the patch in #6. Also the patch contains a number of whitespace issues. MR !18 is the same as MR !15, but on top of the 2.x branch which seems to be the current development branch.
I'm concerned that the related issue referenced in #6 has been open for 8 years.
@phenaproxima - I think handling for a recipe requirement already being in composer.json is accounted for ~ line 116 of RecipeUnpacker and works as you want, though I didn't check on tests.
The MR adds the extra classes, but I am finding it confusing that that we're using menu__item-* and menu__submenu-item-* classes. I think we'd be better off with just menu__item-* menu__item-*--level-X classes on all levels but I think that could break some existing styles out there.
Leaving this unresolved for now.
Looks good to me. (And sorry for earlier mistake.)
You're right about expectException(). I got output like 1 Test, 5 assertions so I thought they were working, but they were not.
sonfd → made their first commit to this issue’s fork.
I started work on the approach proposed in #30 in the 3365328-simple-config-array branch.
It works right now, but the big thing I'm not sure about is: On line ~92, I don't like the weird special handling here to return $value['values'] if it exists. When I was returning the whole value array, array_push() was throwing an error about not allowing unrecognized named variables. My thought is to maybe define the expected parameters in the deriver and then we know exactly what we need to pass to the functions. I think I'll try that next.
sonfd → changed the visibility of the branch 3365328-simple-config-add to hidden.
MR sets the aria-label to "Menu" and defaults the visible label (if enabled) to "Menu".
Updated the button label to "Submenu for LINK_TITLE" and set the default value for the visible label (if enabled) to the same.
I have been thinking about this more and I'm convinced the solution is a recipe. The recipe can:
- Installs Disclosure Menu and Block Breakpoint →
- Creates a "mobile" menu block that's only shown on smaller screens.
- Creates a "desktop" menu block that's only shown on larger screens.
We can then link to the recipe from the project page.
Leaving this postponed for now. In the meantime, this thread can serve as the documentation.
I agree that More {{ item.title }} pages is a verbose, despite it matching the Example Disclosure Navigation Menu with Top-Level Links pattern on w3.org.
It looks like Olivero's submenu toggle is using "Toggle sub-navigation". I don't like that at all. I don't really like "sub-navigation" or "submenu," both feel very jargony to me. I'm inclined to recommend {{ item.title }} pages, but I worry that will be verbose too unless the link title is very short. Something like: "Admission & Aid pages" feels like a lot and considering screen readers would process the menu like "Admission & Aid, Admission & Aid pages" which really feels like a mouthful.
I wonder if More {{ item.title }} is a reasonable compromise even though we'd need to swap to Less {{ item.title }} while it was toggled open.
You make a good point. I'm looking at Olivero's menu toggle and they're using "Toggle Menu" as the aria-label and "Menu" as the visible label. I don't think that the word "Toggle" adds anything useful so I'm good with using just "Menu" for the aria-label and the default value for the visible text.