🇮🇪Ireland @markconroy

Issue summary updated, moving back to 'Needs review'.

---

Thanks to The Confident for sponsoring my time to work on this.

Opening again so we can add more issues/child issues if we need to.

---
Thanks to The Confident for sponsoring my time to work on this.

This was an issue in D8/9 from the Quickedit module.

Let's mark it as outdated.

Thanks @finnsky

I've updated the MR now to use hook_preprocess_layout and added a new library to theme this.

Navigation collapsed:

Navigation expanded:

---
Thanks to The Confident for sponsoring my time to work on this.

@finnsky this looks very interesting.

I wonder should we do it in 2 stages: get the original JS template MR merged, and then follow-up with a proposal to use Web Components?

---

Thanks to The Confident for sponsoring my time to work on this.

MR started - currently just adds the HTML to the block.

I'll try get to the CSS next week.

---
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → made their first commit to this issue’s fork.

Adding patch for latest version of preview link module.

---
Thanks to Big Blue Door for sponsoring my time to work on this.

nod_ → credited markconroy → .

I'd recommend keeping the permissions and the new designs into 2 separated issues so one doesn't block the other.

@ckrina in terms of moving this along quickly, how about we merge this MR "as is" so we have the new navigation module and we also have the "demo purposes" notification. And next week I'll start working on the 📌 Integrate Umami message Active (which I hadn't realised existed until now, sorry).

Or would you prefer me to revert the commits that move the message? If so, it will mean we have tests involving the toolbar even though we don't have toolbar installed any more.

I prefer the first option - get this merged, then do the follow-up.

And (hopefully) finally: Updated the CSS so there is less of it, it's easier to read and we have the same CSS for small screens and large screens

Small screen

Large screen

1. Remove toolbar from modules to enable in demo_umami.info.yml
2. Add navigation to modules to enable in demo_umami.info.yml
3. Move "This site is intended for demonstration purposes" message from toobar to page_top
4. Rename 'toolbar-warning' library and classes to 'demo-profile-warning' and classes
5. Remove 'access toolbar' from permissions in testDemonstrationWarningMessage test
6. Update permissions for author and editor

---
Thanks to Code Enigma for sponsoring my time to work on this.

Thanks for your continued work @pooja_sharma, but we can't uninstall navigation and install toolbar. If we do that we are back to the exact same state that Drupal core is currently in. The point of this issue to to install navigation and to stop using toolbar.

I'll revert your commits.

---
Thanks to Code Enigma for sponsoring my time to work on this.

@pooja_sharma thanks for working on this. I think instead of removing that test, we should have a clause in it that uninstalls the navigation module and enables the toolbar module instead.

Or else we'll need a follow-up issue to place that message somewhere else and update the test to reflect the new position of it.

@smustgrave can you rephrase this please? I'm not sure I follow what you mean.

Think additional resource have to be up the OpenTelemtry tests?

Thanks @RandalV. I'll click around a bit more and see if I can reliably reproduce the issue. Like I said in the summary I'm not even sure how/why it's happening.

Thanks for the quick response.

Adding credit to Open Code for the time to file this issue and work on it.

markconroy → created an issue.

Dammit. There's not such thing as a quick win in Drupal!

Ready for review again.

Toolbar removed.

---
Thanks to Code Enigma for sponsoring my time to work on this.

Assigning credits.

We should account for more space then just 2 active languages. As often sites can have 4-7

I was thinking that as well, though in this specific instance we know we only have 2. But, good point. I'll make a small change tomorrow to give us the exact same experience on mobile and desktop, with all the languages in a vertical list. That will actually be less code and more scalable.

Are you concerned with JS degrade, or just the breakpoint hide/showing.

I have tested this with JS turned off, and it works fine. This is just to make sure that we don't see the button if JS doesn't load, but we do see the links.

Here's a new rewrite of adding an accessible dropdown for the language switcher, which I think is a lot simpler than what we have been trying up to now. Given that no one has worked on this in over 4 years, I think a re-write is warranted, so we can stop the discussion about things not working in IE11, and compiling our ES6 code to ... etc.

Here's my approach:

• Create a template for the language switcher block
• Put a button in that block with the hidden attribute on it, so it's not visible on the page
• Use JS to remove that hidden attribute so we can see the button
• Use JS to add a hidden attribute to the languages so they are now invisible
• Add an eventListener for a click on the button, if clicked toggle its aria-expanded attribute between true/false, then toggle the hidden attribute on the languages correspondingly
• Add a resize function to check the window width
• When the window is resized, if it's below768px do the above items, if not, reset everything
• Update the CSS so we have the following screenshorts - small screen closed languages, small screen open languages, large screen languages

---
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → changed the visibility of the branch 3042417-testing-ignore-this-branch to hidden.

markconroy → changed the visibility of the branch 3042417-accessible-dropdown-for to hidden.

markconroy → created an issue.

Thanks very much @opdavies. Sorry it took me until now to get around to merging it, I thought I had done so waaay back.

--
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → made their first commit to this issue’s fork.

Fixed via a different merge request.

---
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → made their first commit to this issue’s fork.

Thanks @joelseguin I have this merged now.

I think this approach is the correct approach, so there is a double layer before you can edit things

- it must be in the path you are allowed edit and
- you must have the correct permissions to edit it

This means if you have a node of type event inside this path but are not allowed to edit events, we catch that.

Hi @joelseguin

I have a merge request ready for this. Can you test it please?

https://git.drupalcode.org/project/content_access_by_path/-/merge_reques...

The only file you need to look at is the changes in the .module file. All the other changes are just coding standards fixes.

Thanks very much.

---
Thanks to Code Enigma for sponsoring my time to work on this.

Thanks for posting this @joelseguin. I'll work on a fix for it.

---
Thanks to Code Enigma for sponsoring my time to work on this.

I'm going to close this issue as "out of date" since we have separate issues created for each of the requests in this one.

---
Thanks to Code Enigma for sponsoring my time to work on this.

Merge request for this issue: https://git.drupalcode.org/project/drupal/-/merge_requests/8929/diffs

---
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → made their first commit to this issue’s fork.

Thanks @smustgrave, this took me a little bit of time to track down.

I've added a new commit to change the string that we should search for. This is because since we set the nodes to have different dates on them in the new install hook, the string we were searching for belongs to a node that will not make it to the front page listing.

The ordering for that view on the homepage is

Sticky at top of lists => Date added => node id

With the date added now changed, and the node id for this content item being 1, it no longer makes the grade :)

All tests are passing now.

---
Thanks to Code Enigma for sponsoring my time to work on this.

I'm going to mark this as "Needs more info".

We can make the change quite easily if we do it for D11 only, but we'd need to do that fairly quickly before we release D11 in the coming weeks.

Failing that, I think we need more info about why they were moved to the media directory.

---
Thanks to Code Enigma for sponsoring my time to work on this.

Hi @smustgrave,

Looks like we need to keep the line to override the off_canvas library in the $librariesToOverride array.

Tests are passing now.

---
Thanks to Code Enigma for sponsoring my time to work on this.

I've tested this on Firefox and on Chrome on desktop and using the "responsive view" as per the IS instructions and can't find the issue still happening.

Also, all of the screenshots are from the Seven theme, so I'm inclined to think this is a Seven theme issue, rather than a stable9 theme issue.

I'm going to close this as outdated. Feel free to re-open if you can provide steps to reliably reproduce the issue.

I think I've come back out of the rabbit hole and tracked down the correct issue here. It's not that stable9 is trying to reference the wrong files, stable9 does not have any CSS files any more for off_canvas, they were all removed by c2af57548.

So when we try to run the tests, specifically Stable9LibraryOverrideTest, it's looking for CSS files in stable9/css/core/dialog... which are no longer there.

Let's see if removing that section of the stable9.info.yml file and the corresponding library from $librariesToSkip fixes this issue.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Setting to Needs review again, I just want to check that we don't have a red herring test failure.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Also, the patches we have for this so far are for stable and stable9, but stable is not part of Drupal core any more. So at a very minimum we need to update those to refer to stable9 only.

I don't think this is an issue any more.

After installing standard profile, I tried the following tests:

1. Set stable9 as my admin theme, uninstall contact module (via UI). No errors reported.
2. Set stable9 as my admin theme and default theme, uninstall contact module (via UI). No errors reported.
3. Set stable9 as my admin theme and default theme, uninstall Claro and Olivero so only stable9 was left enabled, uninstall contact module (via UI). No errors reported.

If this is still an issue, we will need an updated issue summary and steps to reproduce.

If this is still an issue, I suggest instead of removing this line
The following reason prevents {{ module.module_name }} from being uninstalled:

We look to set {{ module.module_name }} to be a simple variable, so just inside the {% for %}, something like this:

{% for module in modules %}
  {% set module_name = module.module_name %}
{% endfor %}

===
Thanks to Code Enigma for sponsoring my time to work on this.

Thanks @finnsky

I've made some amends to your fixes.

Moved the .footer class back to the <footer> element
Set .layout-footer class to use the CSS that was targetting .footer

I think this is looking pretty good now.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Hi @mgifford

Can you have a glance at this please?

The MR moves tabs/highlighted/content-top/etc all inside the <main> element. And then amends the CSS to target a new container I created inside this called - .main-content-area so that sidebars, etc continue to work.

However, I am not sure if it's best for us to move all those items inside <main> or if, perhaps, we should create a new <section> above <main> and place them in there instead.

Thanks very much!

---
Thanks to Code Enigma for sponsoring my time to work on this.

I'm working on this.

---
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → made their first commit to this issue’s fork.

Thanks @needs-review-queue-bot

Fixed, and setting back to needs review.

Setting to needs review.

I wonder could we do something like a post install update that will cycle through all the nodes on the site and set them as created a day between each.

So if we have 30 nodes, then

• node/30 will be set to have been created yesterday,
• node/29 will be set to have been created 2 days ago,
• node 28 will be set to have been created 3 days ago,
• etc

---
Thanks to Code Enigma for sponsoring my time to work on this.

markconroy → created an issue.

Closing, as no longer relevant.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Closing, as no longer relevant.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Closing, as no longer relevant.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Marking as "Needs review" to let the bots do their dance.

Updating to add credit to Code Enigma.

markconroy → created an issue.

Thanks so much, @b_sharpe.

I'll create a new branch to remove what is currently there and replace it with my version.

Updating credits.

markconroy → created an issue.

At DrupalCon Prague we decided that field--has-multiple-items and field--has-single-item were not communicating the correct information. We need to communicate the field cardinality in the database. When a user sees has-multiple or has-single they will connect this information what is rendered. A multivalue field can have just one entry, but in the original patch it would get a has-multiple-items class.

Just for clarity, the way field.html.twig currently works is that if it's a multivalue field - even if it only has one item in it - it gets the .field__items wrapper. If it has only one item, that item is wrapped in a div and if it has more than one item it uses ul.

The class we are intending to add here field--multiple will denote that this is a multivalue field, not how many items are in the field.
Perhaps we could add another class field--has-N-items to denote how many items we actually have.

Suggestion for classes:

  'field--name-' ~ field_name|clean_class,
  'field--type-' ~ field_type|clean_class,
  'field--label-' ~ label_display,
  multiple ? 'field--multivalue-field' : 'field--single-value-field',
  multiple ? 'field--has' ~ items|length ~ 'items'|clean_class : 'field--has-1-item',
  label_display == 'inline' ? 'clearfix',

Note: we will need to update the issue summary with new notes for the change record.

I think this can be marked as "Closed works as designed", given it's for D7 and hasn't had anyone look at it for over 7 years.

I'm going to close this issue since it belongs to Drupal 9.5 which is no longer supported and our `/core/.stylelint.json` file for D10/D11 does not include the `all` property. in the "order/properties-order" definition.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Hi @YesCT

Do you have steps to reproduce this issue for us?

Also, your commit to add Tugboat looks really valuable. I wonder could you create a separate issue/MR for that, so we can get it merged while this issue is still in progress?

===
Thanks to Code Enigma for sponsoring my time to work on this.

This is a duplicate of 🐛 The "extra_field_block:node:recipe:content_moderation_control" was not found Needs work

===
Thanks to Code Enigma for sponsoring my time to work on this.

Tested againsta 11.x.

LGTM.

===
Thanks to Code Enigma for sponsoring my time to work on this.

Thanks for posting this issue @Lillian Bozeman.

I'll get a fix for it asap.

Adding new patch file.

Adding patch file to use in current projects.

Adding credit to Big Blue Door who sponsored the time to work on this.

markconroy → created an issue.

ekes → credited markconroy → .

This merge request seems to rely heavily on using .toArray(), but I don't think that is supported in Firefox or Safari.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...
https://caniuse.com/?search=toArray

I just want to check that our build tools have something in place to transpile this to FF/Safari-compatible code?

Besides that, we'll done, this is a very large piece of work.

I think this looks like a great idea, but now I also have a worry about us having a field called body and a field called field_body if someone tries to add a new field to a content type and uses the "reuse existing field" approach. Also if you create a new content type it will automatically get the body field added.

I guess people don't need to be creating new fields and content types, since this is a fully formed demo.

I wonder could we disable the default body field from the UI fully?

nod_ → credited markconroy → .

and prevent clicking on a second facet prior to loading the new page.

^ That's a good point.

I'm not sure exactly why we need to disable the checkboxes when we are creating checkboxes from links for facets, so I created this small patch to remove the disabling of the checkboxes and it seems to fix the back button issue.

RTBC!

Let's get this merged!

Hi @xiwar

I tried your branch, but the module wouldn't install from it. Also, I am a little worried without more testing about using Link field. I get that it will work fine for the individual node, but I have a concern about nodes that use the current node as part of their paths without giving it a lot of testing. For example, if you are allowed to edit "Rugby" and the URL is /sport/rugby, will you also be able to edit /sport/rugby/world-cup (as you should be).

In the mean time, I fixed the specific issue we see (thanks for the detailed report), by:

1. Checking if $matches[] has a value,
2. If so, doing the normal thing, and
3. If not, setting NULL as the query value which forces the empty view message to appear. And then I've amended that view message to "Sorry, there is no content assigned for you to edit."

I'm going to mark this as fixed and create a new release. It might be good if you think it's worth it to create a follow-up issue titled "Change paths text field to a link field" and we can work on it from there.

markconroy → changed the visibility of the branch 3418083- to hidden.