UTC-4
Account created on 22 June 2009, about 16 years ago
#

Merge Requests

More

Recent comments

🇨🇦Canada mparker17 UTC-4

My customer has a lot of content (~500,000 nodes), a lot of editors, and a lot of jargon (which is often abbreviated); so they have written a web content style guide to help editors write consistently across all those pages — and the guide says when it makes sense to use an abbr tag (or not).

CKEditor Abbreviation helps my customer because it makes it easy to work with abbr tags without editing the page source, which is particularly useful in some of the longer pieces of content (one particularly popular page has 1 abbr tag amongst ~5900 words, at time-of-writing).

Based on the article you linked, I think it makes sense for people using CKEditor Abbreviation to re-evaluate whether abbr still belongs in their style guides — but even if they decide to remove it from their style guide, they will still need a way to work with it in their old content without editing the source code.

I guess what I'm trying to say is, I think it should be up to the end-users of Drupal to decide whether they should be using the abbr tag — but until CKEditor drops support, and/or the W3C removes/deprecates the abbr tag from the HTML specification, I think the CKEditor Abbreviation module will still be useful for people who have abbr tags in their content (even if it's just to help them remove the abbr tags without editing the page source).

🇨🇦Canada mparker17 UTC-4

@boromino, oops, thank you! All the threads were resolved but I forgot to /approve the merge request. I've done that now. Sorry for the confusion, and thanks for reviewing!

🇨🇦Canada mparker17 UTC-4

I've pushed a commit with an updated use_case; so I'm moving this back to Needs Review. Reviews welcome! Thanks again!

🇨🇦Canada mparker17 UTC-4

@jwilson3, thanks for the quick response and help!

I will update the patch to set $use_case = 18 in the new code.

I tried the other other placement options to see if any worked better, and, unfortunately, they did not (details below).

So we will have to stick with _label_help_append_title() for now. (maybe I can submit a patch to autocomplete_deluxe but my initial attempts at a patch haven't been successful).

To be more specific...

_label_help_prepend_field_prefix($element, $content, $use_case);

The label help text wasn't output at all with prepend_field_prefix

_label_help_prepend_description($element, $content, $use_case);

The label help text appeared below/after the input box and above/before the field description, but this isn't ideal, because I need the help text above/before the input box where users will notice it. Put another way, I could get an effect similar to prepend_description by adding a newline to the description itself, so this doesn't really add anything.

_label_help_append_label_suffix($element, $content, $use_case);

The label help text wasn't output at all with append_label_suffix

🇨🇦Canada mparker17 UTC-4

MR posted; moving to needs review; feedback welcome!

🇨🇦Canada mparker17 UTC-4

I was able to get something to work by adding the following to label_help_process_form()...

      elseif (isset($item['widget']['target_id']['#type']) 
        && $item['widget']['target_id']['#type'] === 'autocomplete_deluxe'
      ) {
        $use_case = 15;
        $element = &$form[$key]['widget']['target_id'];
        _label_help_append_title($element, $content, $use_case);
      }

I'll post a merge request shortly for review, but...

  1. I get the sense from the resulting markup that appending to the title is not ideal, from an accessibility standpoint
  2. I'm not certain if I should change the $use_case to a new number

... some feedback from the label_help maintainers would be ideal.

I'm also happy to submit a patch to the autocomplete_deluxe queue if that would make things easier.

🇨🇦Canada mparker17 UTC-4

We don't have any automated tests for the basicauth, and no specific tests for the standard connector either. Further, search_api_opensearch doesn't have any specific automated tests for its basicauth or standard connectors either.

search_api_solr has test connectors (i.e.: which serve as test doubles in tests), \Drupal\search_api_solr_test\Plugin\SolrConnector\BasicAuthTestSolrCloudConnector, and \Drupal\search_api_solr_test\Plugin\SolrConnector\BasicAuthTestSolrConnector, but the search_api_solr module works in a slightly different way (i.e.: the connectors make the connection to the backend directly; but that's not the case in elasticsearch_connector and search_api_opensearch).

Looking into adding tests for the connectors we have seems non-trivial, so I've created some follow-up issues...

  1. Have ElasticSearchConnectorInterface::getClient() return an Elasticsearch ClientInterface instead of a Client Active
  2. Add tests for the Connectors Postponed

... and I'm going to suggest that we merge this without tests.

🇨🇦Canada mparker17 UTC-4

Here's a patch for the connector types currently in elasticsearch_connector

Postponing as this is a backwards-compatibility break.

🇨🇦Canada mparker17 UTC-4

@sunlix, Yes, please add another commit to clean up package.json!

Thank you very much!

🇨🇦Canada mparker17 UTC-4

For what it's worth, I'm running the updated patch on an 11.2.0 site, and it seems to work fine for me.

🇨🇦Canada mparker17 UTC-4

Trying to debug the source of the errors led me to a fix!

I've reviewed the changes by @sunlix and I'm satisfied with them. I also contributed to the patch (with my Maintainer hat on), and I'm satisfied with my own changes — but another maintainer (@boromino, @sickness29) should review as well.

Moving to RTBC so another maintainer can review!

🇨🇦Canada mparker17 UTC-4

My multiple version testing results...

In the table below:

  • ✅ means that I was able to see the CKEditor on the node/edit page, I was able to enter an abbreviation using the CKEditor Abbreviation button, and the abbreviation worked on the node/view page
  • ❌ means CKEditor did not load on the node/edit page, and thus I was not able to proceed further with testing. I've listed the error shown in the console (but not the stack trace, in part because my JS was minified)

All four sites were set up identically: Standard install profile; CKEditor Abbreviation added to Basic HTML text format; went to /node/add/article to test.

... I'll interpret this in another comment; just wanting to save it here.

🇨🇦Canada mparker17 UTC-4

Also experiencing a BC break in CKEditor Abbreviation: 🐛 Incompatible with Drupal Core 10.5 / CKEditor 45 Active

🇨🇦Canada mparker17 UTC-4

@sunlix, thanks I've updated the issue summary.

🇨🇦Canada mparker17 UTC-4

Maintainer here! Sorry for the delay; it's been a busy few weeks for me.

I have been able to reproduce the problem on Drupal 10.5.0, and can confirm that the code in the patch fixes the issue; but I cannot merge code with broken tests. I also notice some issues in the code: I'll leave some comments in the merge request shortly. For these two reasons I am marking this issue as "Needs Work".

I'll do some more research into compatibility with forward and backward compatibility with Drupal core. There does appear to be some activity in 📌 Update CKEditor 5 to 45.0.0 Active (core), and 🐛 D11.2: Uncaught CKEditorError: Cannot read properties of undefined (reading 'viewUid') Active (editor_advanced_link, another module that integrates with CKEditor) to keep an eye on.

While I was updating the issue summary, I tried to find a link to @ckeditor5/ckeditor5-icons in the NPM Package List, but couldn't. @sunlix, do you have a link to this new package?

🇨🇦Canada mparker17 UTC-4

Turns out I had made an error in #3522107 which made me think i needed this feature; now that I've fixed the error, I don't think this will be necessary anymore. Thanks for your patience with me!

🇨🇦Canada mparker17 UTC-4

(Update issue summary, hopefully to get issue reference to work)

🇨🇦Canada mparker17 UTC-4

Okay, I have created a merge request with a simple solution. Feedback welcome!

🇨🇦Canada mparker17 UTC-4

(Updated the code snippet in the issue summary to use search_api.task_manager to load the tasks by properties)

🇨🇦Canada mparker17 UTC-4

Ah, interesting, I don't think I've used the Menu Multilingual module before... I'll try it out on a test site and see if I can reproduce a problem. Thank you!

🇨🇦Canada mparker17 UTC-4

Since I have asked for someone experiencing this problem help me to figure out how to reproduce the error on a fresh site install, I'm going to change this issue's status to Postponed (maintainer needs more info). If you can help me, please post it here, and change the Status back to Active or Needs work.

(thanks for your understanding and patience with me as I try to manage this module's issue queue)

🇨🇦Canada mparker17 UTC-4

@divya.lakshman,

Apologies for the delay. I created a patch from @berramou in #3085218-20: Filter Menu Tree By Language but it didn't work for me: after installing the right module ( Menu Manipulator ), it crashed my site: see my comment in that ticket. Apparently, the same maintainer (@matthieuscarset) also created Menu Manipulator Sitemap , but that module is independent from this module.

While working on that, I did find a work-around that might work for you depending on your site...

  1. Go to /admin/config/regional/content-language. At the top of the page, check Custom menu link to make them translatable. This will cause a Custom menu link details-element to appear further down the page. You can leave most configuration in that Custom menu link details element at their default values, but I unchecked "Changed" because it didn't seem like that needed to be translatable for my tests. I clicked Save configuration on that page.
  2. Go to /admin/structure/menu, edit a menu, and edit a menu link. Click the Translate tab at the top of the page. Click Add next to the language(s) that you want to translate that menu link into. Enter a translation and save.
  3. Add translations for a handful of menu items for testing. You will want to repeat for all menu items eventually.
  4. Go to /sitemap and use the language switcher. You should see the translated text for the menu items that you entered translations for change based on the current language.

This work-around might be sufficient for your use-case if:

  1. All of your menu links are custom menu links (i.e.: they were added manually, i.e.: they are not added by a module).
  2. You want exactly the same menu items in exactly the same order for all translations.

To make it a bit easier for me to manage this module's issue queue, I'm going to change this issue's status to Postponed (maintainer needs more info), but you can change it back...

If the patch in #3085218-20: Filter Menu Tree By Language works for you, please let me know by posting that here, and change this issue's status to Fixed

If 3085218 seems unrelated, then please post that here, change this issue's status back to Active, and I will try to help further (I'll probably ask for steps to reproduce!)

If you figure out a solution to your problem (i.e.: Menu Manipulator Sitemap works for you, or my Content-translation work-around works for you), please post that here, and change this issue's status to Fixed.

If I don't hear back from you in ~6 months or so, then I will close this issue as Closed (cannot reproduce) to make it easier for me to manage the other issues in this issue queue.

Thanks for your understanding!

🇨🇦Canada mparker17 UTC-4

For people who prefer patches, I've created a patch of the current state of the merge request.

(if you want to contribute code to this issue please contribute in the merge request: I'm only providing these patches as a convenience this time, to help me answer a question in another issue — thanks for your understanding)

🇨🇦Canada mparker17 UTC-4

I've converted the patch in #14 into a merge request, and I made the changes I requested in the first part of #16, but, when I installed menu_manipulator-3.1.0 or menu_manipulator-4.0.0 (I tested both), when I go to /sitemap, I get a WSOD, and the error...

InvalidArgumentException: Class "menu.language_tree_manipulator" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (line 32 of core/lib/Drupal/Core/DependencyInjection/ClassResolver.php).

... and I'm not sure where to go from here, because I don't think I understand the Steps to Reproduce!

Here's what I did to test...

I started by setting up a test environment.

  1. Installed ddev-1.24.6
  2. git clone --branch '8.x-2.x' https://git.drupalcode.org/project/sitemap.git && cd sitemap — cloned this module
  3. git remote add sitemap-3085218 https://git.drupalcode.org/issue/sitemap-3085218.git && git fetch sitemap-3085218 && git checkout -b '3085218-filter-menu-tree-by-language' --track sitemap-3085218/'3085218-filter-menu-tree-by-language' — switched to this issue's branch
  4. ddev config --project-type=drupal --docroot=web --php-version=8.3 --corepack-enable --project-name=sitemap — set up a ddev site for testing the module in isolation
  5. ddev add-on get ddev/ddev-drupal-contrib — installed the very-handy DDEV Drupal Contrib add-on
  6. ddev start
  7. Edited composer.json, added "drupal/menu_manipulator": "^4" (later I tried ^3) to the require-dev section for testing purposes
  8. ddev poser — run composer-install as per the ddev-drupal-contrib setup instructions
  9. ddev symlink-project — link the module into place as per the ddev-drupal-contrib instructions
  10. ddev drush -y si demo_umami — install Drupal with the Umami Food Magazine demo content — normally I use the minimal install profile, but I know the Umami demo is a multilingual site, so I figured it would be a good test case for this issue.
  11. ddev drush -y en sitemap — enable the sitemap module
  12. ddev drush -y uli — get a login link; pasted in browser

Then I set up the site...

  1. I went to /en/admin/config/search/sitemap and enabled the Menu: Main navigation and left its configuration at the defaults.
  2. I went to /en/admin/structure/menu/manage/main. I saw that the Menu language = English. It struck me that maybe you're intended to use entirely different menus in each language, but the issue summary and discussion in this ticket seemed to imply that there were menu items for more than one language in the same menu, so I went to figure out how to do that.
  3. I checked the Translate menu tab, but I could only translate the title and administrative summary of the menu itself, i.e.: not the menu items.
  4. Editing the items in the menu, I saw they were "special" (i.e.: managed programmatically), so I disabled them (i.e.: by unchecking them and clicking "Save"). I then re-created custom menu links to the same locations (i.e.: <front>, /en/articles, and /en/recipes).
  5. While creating/editing my new custom menu links, I did not see a language selector, (nor a Translate primary action), so it was unclear to me how to get menu links from multiple languages in the same menu.
  6. I know menu links are content entities, so I went to /en/admin/config/regional/content-language to check out the settings there. The Custom menu link entity-type was not set to be translatable, so I checked it to make it translatable. I left most configuration in the Custom menu link details element at the default values, but I unchecked "Changed" because it didn't seem like that needed to be translatable. I clicked Save configuration on that page.
  7. I went back to /en/admin/structure/menu/manage/main and edited a custom menu link. Now I could see the language selector. I also saw a Translate tab.
  8. To see what would happen, I used the Translate tab to add a Spanish translation for some of the menu links. Then I refreshed /en/sitemap, and /es/sitemap in a separate tab. To my surprise, I saw the English translations for the menu items on the English sitemap, and the Spanish translations for the menu items on the Spanish sitemap. It strikes me that making Custom Menu Links translatable, and adding translations for them might be a more-sustainable work-around for some sites. That being said, I'm aware of long-standing bug 🐛 Untranslated menu items are displayed in menus Needs work , and I have also worked on highly-localized sites where there completely different content in different languages, down to different menu structures and orders, so I'm aware this won't work for everyone.
  9. I could now add menu items that were only in Spanish, and could see them on both the English and Spanish sitemaps.
  10. I then went to /en/admin/modules and enabled the Menu Manipulator module.
  11. Since I had already the new code in place (i.e.: from step 3), I decided to refresh /en/sitemap, and /es/sitemap to see if it made the untranslated menu links go away. This is when I got the InvalidArgumentException: Class "menu.language_tree_manipulator" does not exist error mentioned at the start of this comment.
  12. I tried going to /en/admin/config/user-interface/menu-manipulator to see if there were any settings to change, but I got a different WSOD, ArgumentCountError: Too few arguments to function Drupal\Core\Form\ConfigFormBase::__construct(), 1 passed in /var/www/html/web/modules/contrib/menu_manipulator/src/Form/MenuManipulatorSettingsForm.php on line 88 and exactly 2 expected in Drupal\Core\Form\ConfigFormBase->__construct() (line 44 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

Now I'm not sure what to do next!

Can someone experiencing this problem help me to figure out how to reproduce the error on a fresh site install?

🇨🇦Canada mparker17 UTC-4

Hi Divya, thanks for reporting this!

The issue that you described sounds a bit like the issue reported in Filter Menu Tree By Language Needs work

The patch in 3085218 needs to be updated before i can recommend that you try it. If you feel comfortable updating the patch yourself, that would be great! But if you would prefer, i can update the patch later today and let you know when it is ready to try.

🇨🇦Canada mparker17 UTC-4

Update the issue summary to document changes made in this ticket.

🇨🇦Canada mparker17 UTC-4

Update the issue summary to document changes made in this ticket.

🇨🇦Canada mparker17 UTC-4

Update the issue summary to document changes made in this ticket.

🇨🇦Canada mparker17 UTC-4

Update the issue summary to document changes made in this ticket.

🇨🇦Canada mparker17 UTC-4

Update the issue summary to document changes made in this ticket.

🇨🇦Canada mparker17 UTC-4

Update issue summary to note release version, explain API changes.

🇨🇦Canada mparker17 UTC-4

Updating issue summary to make it easier for people to see what changed.

🇨🇦Canada mparker17 UTC-4

(Update issue summary to mention release version)

🇨🇦Canada mparker17 UTC-4

(update remaining tasks in issue summary to include release)

🇨🇦Canada mparker17 UTC-4

Accidentally said it was released in 8.0.0-alpha4 when it was actually released in 8.0.0-alpha3

🇨🇦Canada mparker17 UTC-4

Updated remaining tasks to show when this issue was released.

🇨🇦Canada mparker17 UTC-4

Updated remaining tasks to show when this issue was released.

🇨🇦Canada mparker17 UTC-4

Updated steps to complete so I can say when it is released.

🇨🇦Canada mparker17 UTC-4

Updated issue summary to mention when it was published.

🇨🇦Canada mparker17 UTC-4

Updated issue summary to mention when it was released.

🇨🇦Canada mparker17 UTC-4

@marckwee, thank you for the contribution! This sounds like a great idea to me!

I committed your code to a merge request to make it easier for me to review. I took a very quick look at your patch: the changes seem clear to me, but I haven't yet had a chance to run the changed code... I may have more feedback when I do.

We should add a change record to assist people upgrading from 8.x-7.x

I didn't see any automated tests in your patch... we prefer to accept merge requests that have passing automated tests. Automated tests ultimately benefit you, because they ensure that future changes to this module (i.e.: by other people) will not break the functionality that your site depends on!

🇨🇦Canada mparker17 UTC-4

@marckwee, thank you for the contribution!

I committed your code to a merge request to make it easier for me to review.

I took a very quick look at your patch: the changes seem clear to me, but I haven't yet had a chance to run the changed code... I may have more feedback when I do.

I didn't see any automated tests in your patch... the Elasticsearch Connector module maintainers prefer to accept merge requests that have passing automated tests. Automated tests ultimately benefit you, because they ensure that future changes to this module (i.e.: by other people) will not break the functionality that your site depends on!

I don't think that we have any automated tests for Facets yet ( I wrote a manual test but haven't had the time to turn it into code yet).

Are you interested in learning how to write an automated test?

If you describe how to run a test manually, then I may be able to suggest how to automate it (i.e.: if you're interested in learning to write a test), or write a test myself (if you aren't interested in learning to write a test).

🇨🇦Canada mparker17 UTC-4

I think that, since we've split the remaining tasks into 📌 Change sitemap.settings.plugins.*.weight from type integer to weight Postponed and 📌 Start using #config_target in SitemapSettingsForm Active , we can mark this as fixed.

🇨🇦Canada mparker17 UTC-4

I split off...

3. If we dropped support for D9 and D10.0 through 10.1 (i.e.: support 10.2+), we could simplify the configuration form by making use of #config_target in our main config form.

... into its own issue, 📌 Start using #config_target in SitemapSettingsForm Active ; updating the issue summary to say this.

🇨🇦Canada mparker17 UTC-4

mparker17 changed the visibility of the branch 3465601-config-schema-2 to hidden.

🇨🇦Canada mparker17 UTC-4

I split off...

2. If we dropped support for D9 and D10.0 through 10.2, (i.e.: support only D10.3+ and D11.0+ only, we could change sitemap.settings.plugins.*.weight from type integer to weight

... into its own issue, 📌 Change sitemap.settings.plugins.*.weight from type integer to weight Postponed ; updating issue summary to say this.

🇨🇦Canada mparker17 UTC-4

Updating the issue summary: we are unblocked now, so moving to Needs Work.

🇨🇦Canada mparker17 UTC-4

Updated issue summary to note we committed the patch and released it in 8.x-2.0-rc1 .

🇨🇦Canada mparker17 UTC-4

I merged Add an option for menu depth Needs review and 📌 Use attributes instead of annotations Active today, so I rebased this branch on top of the latest 8.x-2.x.

🇨🇦Canada mparker17 UTC-4

Updating Issue Summary's Remaining tasks.

🇨🇦Canada mparker17 UTC-4

I made a few small tweaks, but otherwise, everything looks good; and manual and automated testing show everything is working great, so I'm going to merge this! Thanks everyone!

🇨🇦Canada mparker17 UTC-4

Marked as Fixed.

Updated the issue summary with more information, for users of the module who are interested in the change when they find it in the release notes.

🇨🇦Canada mparker17 UTC-4

Speaking of PHP versions and attributes, I think it's best to explicitly add a PHP requirement (>=8.1) in composer.json.

Makes sense: I've done that in the latest commit.

I expected the Drupal.org Composer Service (façade) a.k.a. project_composer to update composer.json with a PHP version to match what's in sitemap.info.yml, but to my surprise, I couldn't find any code to do that, so I updated both composer.json and sitemap.info.yml.

If tests pass, then I'm going to merge this. Thanks!

🇨🇦Canada mparker17 UTC-4

Is this still an issue?

(thanks for your patience with me: I am reviewing old issues to try to keep this module's issue queue in a manageable state)

🇨🇦Canada mparker17 UTC-4

I think this would be a helpful feature! But I'm not sure how I would implement this, I don't foresee my clients needing this anytime soon, and I'm not sure I have the volunteer time right now, so contributions would definitely be welcome!

🇨🇦Canada mparker17 UTC-4

I think I'd prefer to have a separate issue for the translatable taxonomy term names, instead of an omnibus issue (especially since one of the items in this issue is blocked by a core issue), so I have created [PP-1] Support translatable taxonomy term names once TermStorage::loadTree is language-aware Postponed and credited @akalata, @bgilhome, @flocondetoile, and @miiimooo for their work on this issue. I'm going to move this issue back to its previous state. Thanks for your understanding and patience with me!

🇨🇦Canada mparker17 UTC-4

@julio.raimondi, thank you for filing this feature request!

I think that optional support for the Translatable menu link uri module would be a great addition to the Sitemap module, but I have no experience working with it, my clients aren't planning to use it anytime soon, and I'm not sure I have the volunteer time right now, so contributions would definitely be welcome!

(aside: removing the non-standard tags as per the issue tag guidelines )

🇨🇦Canada mparker17 UTC-4

Note (mostly to myself): I found Display menu hierarchy as org chart? Active requesting something that sounds similar. I've requested that the person who filed that issue (@damienmckenna) comment here if they think there is some overlap between what they requested and what was built here.

🇨🇦Canada mparker17 UTC-4

@damienmckenna, recently, @malcomio filed a new ticket with a similar goal — Provide option to display as a visual sitemap Active — and provided some code.

Would #3525996 suit your use-case? If so, would you be willing to provide feedback on that ticket?

🇨🇦Canada mparker17 UTC-4

This issue needs my review, so I'm going to move this to RTBC to remind myself to do that. Sorry for the delay; thanks for your patience!

🇨🇦Canada mparker17 UTC-4

This issue has been Postponed (maintainer needs more info) for 10 months and no new info has been posted, so I'm going to move this issue to Closed (cannot reproduce) for now.

That being said, if you have more information, feel free to re-open the issue!

Thanks for your understanding and patience with me!

🇨🇦Canada mparker17 UTC-4

@berramou, thank you for the excellent contributions! The code is clear, it makes good use of APIs, and doesn't miss anything as far as I can tell. Manual testing shows everything working as far as I can tell. I don't think this needs any new tests, because the existing tests cover the changes (and they all pass!).

I've rebased this onto 8.x-2.x.

Quick question before I merge this: I noticed that in some annotations in core (for example, in the handlers sub-array of the ConfigEntityType annotation on \Drupal\block\Entity\Block), references to other classes in annotations are done with the special ::class constant... should we also do that for Sitemap's deriver annotation-keys in the following places?

  1. modules/sitemap_book/src/Plugin/Sitemap/Book.php,
  2. src/Plugin/Sitemap/Menu.php,
  3. src/Plugin/Sitemap/Vocabulary.php, and
  4. tests/modules/sitemap_custom_plugin_test/src/Plugin/Sitemap/DerivativeSitemapPlugin.php

... for example, in src/Plugin/Sitemap/Menu.php, should we modify the code as follows...

 use Drupal\sitemap\Attribute\Sitemap;
+use Drupal\sitemap\Plugin\Derivative\MenuSitemapDeriver;
 use Drupal\sitemap\SitemapBase;
 /*...*/
 #[Sitemap(
   /*...*/
-  deriver: "Drupal\sitemap\Plugin\Derivative\MenuSitemapDeriver",
+  deriver: MenuSitemapDeriver::class,
   menu: "",
 )]
🇨🇦Canada mparker17 UTC-4

mparker17 made their first commit to this issue’s fork.

🇨🇦Canada mparker17 UTC-4

(I promised to move the issue to Needs Work in the previous comment but forgot to select the State)

🇨🇦Canada mparker17 UTC-4

@malcomio, thank you for the contribution!

Later, I will do a more-in-depth code review, and test the new functionality; and I may have some more feedback at that time. For now, I took a very very brief look at the merge request, and noticed some things...

  1. cspell doesn't recognize the word brailsford's - you can fix this by adding the word to .cspell-project-words.txt
    1. For more information, see the CSpell on Drupal.org documentation.
  2. stylelint has raised 66 CSS style issues - you can fix many of them automatically by passing --fix to stylelint.
    1. Aside: I use ddev/ddev-drupal-contrib to make stylelint available in my local development environment - with that running, the command ddev stylelint --fix should fix most issues.
  3. I didn't see any automated tests in the merge request... the Sitemap maintainers prefer to accept merge requests that have passing automated tests. If you need help writing tests, please ask!
    1. I think you could check that the CSS file is included when the new option is checked; and not-included when the option is unchecked. tests/src/Functional/SitemapCssTest.php does something similar for sitemap.theme.css.
    2. Automated tests ultimately benefit you, because they ensure that future changes to this module (i.e.: by other people) will not break the functionality that your site depends on!

I also have some deeper questions...

Item 4 (i.e.: continuing from the ordered list above)

Regarding the use of Matt Brailsford's CSS Sitemap, I don't see a license in that repo. For better or worse, we are only allowed to host code that explicitly has the GPL 2.0+ license on drupal.org (and, even if we find a project whose license is compatible, when we copy that other project's code into the Sitemap project, it becomes the Sitemap module maintainers' job to monitor the upstream repo and keep Sitemap module's copy up-to-date).

The recommended way around this is to use the Libraries module , i.e.: declare the third-party code as a library, rely on the site's maintainer to download the library, and use the Libraries module to link to it properly.

Item 5

In the issue description and comment #2, you gave ~5 different visual sitemap options (i.e.: GlooMaps-style, Canva-style, MermaidJS-style, Slickmap-style, and Matt Brailsford's CSS Sitemap), but you didn't really explain why you chose Matt Brailsford's CSS Sitemap to be the default.

For what it's worth, I think Matt Brailsford's CSS Sitemap looks nice — but as the module's maintainer, I have to explain to the other 36,000 sitemap-2.x users why we picked it over the other 4 options presented here.

One way that we could make the decision to pick a default easier for the other sitemap-2.x users to accept is by making it easier for them to use their own (alternative) visual style. We could do this in several ways:

  1. Update the module documentation to explain how to use the libraries-override directive in their theme to change the visual style.
    1. This is definitely the easiest option, but I daresay we might have to be prepared to answer the question "why didn't we use libraries-override to override the existing sitemap.theme library instead of adding a separate visual style?"
  2. Change the configuration key from visual_css to something like visual_css_matt_brailsford, to make it easier for users to see how they could patch in their own visual_css_gloomaps, visual_css_canva, etc.
  3. Provide an API for other modules to easily register their own visual css library and select it on the sitemap options page.
    1. This is definitely the most-complex option; but would allow people with other visual sitemap preferences to add their own sitemap_mermaidjs, sitemap_slickmap, etc. modules to the Sitemap module's ecosystem (and/or easily create a custom module for their own site theme).

Pending answers to these deep questions; fixes for the cspell and stylelint lints; and automated tests, I am going to move this to "Needs work". That being said, please me know if there is anything that I can do to help!

🇨🇦Canada mparker17 UTC-4

Thanks everyone for your hard work on this issue!

I'm taking a look at the merge request and trying to understand what it does...

I don't think I've run into a Data Source plugin before, so please forgive me for the silly question (and for using Elasticsearch-specific terms), but does a Data Source plugin tell Search API how to work with an Index with a dynamic mapping and/or an Index whose content is ingested by third-party crawler (e.g.: Elastic web crawler or Apache Nutch)?

Production build 0.71.5 2024