- Issue created by @malcomio
- 🇬🇧United Kingdom malcomio
- Merge request !41Issue #3525996: Provide option to display as a visual sitemap → (Open) created by malcomio
- 🇨🇦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...
- cspell doesn't recognize the word
brailsford's
- you can fix this by adding the word to.cspell-project-words.txt
- For more information, see the CSpell on Drupal.org documentation.
- stylelint has raised 66 CSS style issues - you can fix many of them automatically by passing
--fix
to stylelint.- 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.
- Aside: I use ddev/ddev-drupal-contrib to make stylelint available in my local development environment - with that running, the command
- 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!
- 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 forsitemap.theme.css
. - 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 think you could check that the CSS file is included when the new option is checked; and not-included when the option is unchecked.
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:
- Update the module documentation to explain how to use the
libraries-override →
directive in their theme to change the visual style.
- 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 existingsitemap.theme
library instead of adding a separate visual style?"
- This is definitely the easiest option, but I daresay we might have to be prepared to answer the question "why didn't we use
- Change the configuration key from
visual_css
to something likevisual_css_matt_brailsford
, to make it easier for users to see how they could patch in their ownvisual_css_gloomaps
,visual_css_canva
, etc. - Provide an API for other modules to easily register their own visual css library and select it on the sitemap options page.
- 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).
- This is definitely the most-complex option; but would allow people with other visual sitemap preferences to add their own
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!
- cspell doesn't recognize the word
- 🇨🇦Canada mparker17 UTC-4
(I promised to move the issue to Needs Work in the previous comment but forgot to select the State)
- 🇬🇧United Kingdom malcomio
Thanks for the detailed response, and the suggestion of ddev-drupal-contrib - I hadn't seen that before.
I'll look at the various points you raised.
In terms of why I chose Matt Brailsford's CSS, it was just the most visually appealing to me, and technically simplest.
It did need some modification to work with Drupal, because it assumed particular markup.
It is itself a derivative of Slickmap, which uses The Unlicense.
So perhaps it would make sense to alter the styles so that they aren't based on Matt Brailsford's CSS.
In terms of an API for registering a different library, that feels like overkill to me - if a particular site wants to override styles, they can do it in the theme layer.