rkoller → created an issue.
nice looks good! one potential problem, one that the coffee module has as well and another unrelated observation.
the problem the coffee module has is how do people discover the actual hotkey (that one is in place)? like the tour module the coffee module also has a top level menu item. but that the coffee module has a hotkey can easily be missed. i'Ve missed and overread that fact on the project page for a long long time. so i wonder would it make sense for one to also add a note about the hotkey to the project description of the tour module on d.o anyway (some people actual read that thoroughly) but i also wonder if it would make sense to add one tip to one of the tours informing the user? maybe not in the tours tour which is only about the tour admin interface but on a tour people use for onboarding? not sure what the best place would be tbh.
and on unrelated note. i've noticed i am able to jump between tour tips by pressing the arrow left and arrow right key? that was sort of unexpected. there is no indication nor visual feedback in the ui. the tip just changes. but i am also unsure how you could indicate that button press and change? (but out of the scope for this issue even unsure if it is worth an issue right now. it was just unexpected and everything looks so static "arrowing" through the tip with no feedback except the new highlights and text)
at the moment the only hotkey i know in the context of drupal is the one set by the coffee module (alt d). so maybe using the alt key in combination with "t" might be an option? i just dont like something like control and alt. plus that might cause conflicts on the mac with voiceover. the voiceoverkey is ctrl + alt.
and hotkeys might be something that could be improved for drupal core in general. something i've noticed in the competitive review where we took a look at different CMSes in the context of a11y and the features they have. joomla for example has a directly accessible hotkey menu listing the most important keyboard shortcuts. quite handy.
if you have the following content related fields on the tip editing page
label
alt text
body
Personally i think simply a <span class="visually-hidden">alt text</span>
could be added in front of the body field. so the screen reader user first gets the "alt text" describing the circumstance providing the necessary context only sighted users have otherwise and then the actual tip is announced. but i'll raise the topic on the next weekly a11y starshot meeting and ask the others about their opinion. cuz the downside on second thought might be that by adding just a visually hiddens span it might be the case that the alt text content wouldnt be skipable. so even though it would be very helpful to provide the context with the alt text but the downside as soon as you have an experienced drupal user the alt text might be obsolete and uninteresting and the actual tip might be the sole point of interest.
hmmm to be honest for me the second sentence is not clear:
**Disclaimer: Due to Tours being config and possible to be edited on a site basis we will not be doing any Tour or Tip updates retroactively. All updates can be made but no update hook will be added.**
what does "all updates can be made" imply? updates within the ui or programmatic updates? i not really understand the actual implications. therefore i am also uncertain about suggesting an alternative wording.
and since you reworded the title and removed the helper function. i would still have the question i really struggled with a few months ago while the tour module was still in core which might be relevant to this issue. back then we started creating tours for eca. but the problem was with the tour module already installed and eca already installed and with tours being added to eca manually (during the creation) or when the tours are ready in a new version of eca - those tours were unavailable in the tour module (only the yaml files were in place). i would have to uninstall eca and reinstall it (or alternatively tours?) to get the tours available in the tour module which is sort of inconvenient. but havent tested nor worked on these tours in a while, sort of stalled back then. but a case like that i consider still relevant?
with voiceover i've tested with safari (see and listen in the video), firefox and edge on macos and the dialog popup is announced properly across all browsers. looks good!
in regards of the upstream issue. do you mean opening an issue in regards of aria-haspopup there as well or do you refer to the dialog element topic? for the latter i've already opened an issue: https://github.com/shepherd-pro/shepherd/issues/2959. i am just battling with the shepherd environment run within ddev (the problems are also mentioned on the issue). otherwise i would already tried opening a pull request (or at least tried if i am able to change the markup to a dialog element locally in a first step).
Left a comment on the issue: #3459246-53: Add a '#check_all' option for checkboxes element →
Usability review
We discussed this issue at 📌 Drupal Usability Meeting 2024-09-06 Needs work . That issue will have a link to a recording of the meeting. For the record, the attendees at today's usability meeting were @benjifisher, @rkoller, @simohell, and @worldlinemine.
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
We have taken a look at the current state of MR8985. Overall we had the consensus that adding a check-all option would be a good thing. First a few details we’ve noticed during our explorations:
- Completely out of scope: While setting up the select field, we’ve realized it is not communicated anywhere in the admin UI how to choose between checkboxes or radio buttons. The option on the form display widget is simply called “Check boxes/radio buttons”, but it is not communicated anywhere that things are controlled via the cardinality/“allowed number of values”-setting on the field settings page. A cardinality limited to one switches to radio buttons, while anything bigger than one or
Unlimited
switches to checkboxes. - On a related note relevant to this issue, if you set the allowed number of values to one for the select field, the
Show "Check all / none” button
checkbox or better the form display widget settings in general is shown anyway, even though it is irrelevant without any purpose in this context. - The check-all button has a too low color contrast (#d3d4d9 against #ffffff => 1.5:1 -> 3:1 necessary), but there is already an issue for Core: 🐛 Grey button's background color has a too low contrast ratio against page background Needs review .
- The button is no toggle button, so the current as well as the future state is unknown which is potentially confusing for screenreader users (see voiceover.mp4). Even though
aria-pressed
is supporting tri-state with offering themixed
state (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attribut...), the support across screenreaders and browsers is subpar: https://a11ysupport.io/tech/aria/aria-pressed_attribute. Plus there is no component to communicate a pressed button state in the Drupal Design System at this point, and then there would also be the need for a “mixed” button state as well. And how to communicate such an intermediate state for a button visually? - With the button state unknown, the button label adds another level of confusion - it contains both possible future states within the
Check all/none
label and readability wise it is sort hard to process with the slash separating the two possible states . For sighted users with the visual context, it might make them think for a second, but after using that pattern once it is manageable. Screenreader users instead don’t have the state nor the visual context available, just based on the button label they don’t know if pressing the button will check or uncheck everything and which of the available options is already ticked? Strictly speaking a screenreader user would have to check every available option before going back to the check-all button. The only shortcut might be by pressing the button, check the first option and based on its checkbox state go back to button in case the state is the opposite of the desired state. And switching the button label based on the state as suggested in #50 → has also its downsides for screenreader users as described for the “show and hide password” pattern by Leonie Watson in this webinar by the Smashing Magazine: https://youtu.be/OUDV1gqs9GA?si=b4cf_sUtRiF5DJ1f&t=3220 - Visually, the button and the set of checkboxes underneath feel disconnected, it feels like two separate sets of controls, one button and a set of eight checkboxes that are combined underneath a headline. Despite the spatial proximity it does not seem that the two are one functional unit.
After we’ve gained an initial overview we’ve turned towards the fundamental question, which of the two options should be selected, the button or the checkbox based pattern? In the run-up to the meeting I’ve done some research looking for resources about the check-all button and check-all checkbox pattern in the context of UX related resources. It has to be noted that all links in the following are about checkboxes, I was unable to find any article nor example about the check-all button pattern:
https://ux.stackexchange.com/questions/98089/nested-checkbox-behavior-ch...
https://www.nngroup.com/articles/checkboxes-design-guidelines/
https://app.uxcel.com/courses/ui-components-n-patterns/selection-control...
https://css-tricks.com/indeterminate-checkboxes/
https://uxdesign.cc/selection-controls-ui-component-series-3badc0bdb546
https://blog.logrocket.com/ux-design/checkbox-ui-design-best-practices-e...
https://forum.figma.com/t/select-all-checkbox/48623/5
https://www.setproduct.com/blog/checkbox-ui-design
Then there are few examples of design system that are using the nested checkbox list pattern described in the aforementioned list of resources:
https://carbondesignsystem.com/components/checkbox/usage/
https://m2.material.io/components/checkboxes
https://atlassian.design/components/checkbox/examples
So overall the nested checkbox list pattern seems to be the solution of choice from an UX perspective. This also corresponds to the accessibility perspective reflected in the following resources:
https://adrianroselli.com/2024/03/check-all-expand-all-controls.html
https://adrianroselli.com/demos/do-undo-all/
https://dequeuniversity.com/library/aria/checkbox-tri
https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/examples/checkbox-mixed/
(*the behavior in the w3.org example is odd and confusing since it remembers which checkboxes of the children were ticked for the indeterminate state across checkbox presses)
At the moment, the favorite and preferred option for solving this issue is to go with a check-all button, according to #50 → . But based on the fact that the majority of resources we've found during the research are utilizing checkboxes to check other checkboxes, Drupal already using check-all checkboxes in other contexts within the admin UI, and us trying to keep the admin UI consistent not using different interface components for the same kind of functionality (see #51 → ). Unfortunately all these points are not in line with a check-all button. :( So we would recommend the following points:
- If the allowed number of fields is set to
1
on the field settings page of a select field, hide the settings for theCheck boxes/radio buttons
option on the corresponding form display widget. - Replace the check-all button with a checkbox. Visually distinguish the check-all checkbox from the checkboxes underneath by either indentation seen in the various link examples above or by adding a separation line/border.
- Replace
Show "Check all / none" button
withSelect all
as the label for the check-all checkbox. - Open a follow-up issue that is adding the tri-state checkbox pattern for Core in general. So an
intermediate
state is added to the already existingchecked
andunchecked
state for check-all checkboxes. Aside select fields on node edit forms we have found the following other places that are using this check-all checkbox pattern (there might be even more):admin/content
,admin/content/comment
,admin/content/media
,admin/content/media-grid
,admin/people
, and on a content moderation workflow page within the dialog modal forOperations
.
For the point mentioned in the beginning which is out of the scope for this issue I will open up a new issue. I'll remove the "needs usability review" tag and set the issue back to "needs work".
i agree with simo. the only problem, on admin/content
the "check all"-checkbox is placed in the table header. so it is not directly applicable to the pattern used on a list field. I've done some research in preparation when we find time to discuss the issue in ux meeting. Hope we get to the issue tomorrow.
yay! that fixed the problem. now i am getting the disabled button on nodes that dont match the route for the tour and the console error is gone, while for the node that is matching the route the tour button is still shown and the tour is starting properly. tested in safari and edge. thanks
Moving over from last week:
✨
Add a '#selectall' option for checkboxes element
Active
✨
Expose triggering update of media metadata + thumbnail to end users
Needs work
and adding ✨ Add a Title Formatter Active raised by @smustgrave on the ux channel
It is not resolved. i had to rebuild my setup today due to a problem in project browser that broke the entire instance. so i had to set up everything again. for anonymous users i am unable to reproduce right now but with my admin user instead. but there is a detail i havent mentioned in the issue summary (unsure if it was the case when i ran into the error the first time). anyway my setup on a fresh instance:
1) gave all available roles, anonymous included, the permission to view tours.
2) created a new tour with the route
entity.node.canonical
- node:2
and added one tip
3) added the tour button block to the social bar in olivero
4) created two nodes
5) then accessed the frontpage with admin in one window and with anon in the other. with anon i am unable to run into the error but with the admin user on the page for node 1 when i click the take a tour of this page button the second time after nothing happens the first i run into the error. the front page shows correctly the no tour available button and node 2 shows the actual tour when i click the take a tour button.
it feels like in combination with that route
entity.node.canonical
- node:2
there is something off with showing the button in the correct state every time. and when the button is shown in the wrong state then clicking the button (which should be disabled and not enabled is causing the error? added a short recording of the output (with anon which i havent added to the video there are no errors right now anymore)
I've checked the two pages in question as well. In regards of the settings in chrome and edge. i have ticked the "enable automatic dark mode" checkbox and the "Prefers contrast more" in addition to just "forced colors: active" which mimics the windows high contrast mode. In the following a list of issues on the browse and details page.
Browse Page
- Underline for the active browse tab has a too low color contrast (but that looks like a problem for claro in general?
- Underline for the active source type has a grey underline instead of darkish yellow, which is slightly higher contrast but still under 3:1
- strictly speaking the border of the search field and the select fields in the filter field set, with 2.9:1 with rgba(87, 87, 87, 1) against the black background is a slightly too small color contrast.
- the icons in the list and grid button are invisible
- the categories have a border that is none existent in the none high contrast mode
- the border of the add and install button has the same border color problem like the search field and select fields and a color contrast of 2.9:1
- the outline circle for the active page in the pager has a too low color contrast
- in the page first, previous, next and last icon is close to invisible
- the spinner when installing a module is also greyish with a probably too low color contrast. in regular mode the spinner is blue maybe in high contrast mode make it simply white?
*i will continue the list for the detail page. but while installing a second module i broke my install again. got the installer page issue another time. i will assign the issue to me for now until the write up is done.
We've agreed on slack to make this issue a meta issue and chop this issue up into smaller child issues.
aside the aspect of providing an opportunity to encrypt sensitive data, another important point to mention and maybe to consider might be the creation of technical organizational measures (TOMs).
https://thegdprcomplianceconsultancy.co.uk/the-meaning-of-technical-and-...
https://aigner-business-solutions.com/en/blog/gdpr-explained-simply-toms...
The scope of TOMs is definitely broader than just the CMS, but it might be worth to consider the option to autogenerate a TOMs template for a site. It is impossible to autogenerate a complete TOMs template but based on the sites config you already know the technical measure in place for the drupal site (which modules and configuration are used and which process are defined, and for example certain workflows in eca that have processes in place that are relevant to the gdpr, same as content moderation workflows and so forth). the other relevant points for the TOMs could be left blank and the user is able to fill those blanks. so the user has a template speeding up the generation of the initial TOMs as well as making it easier to keep those up to date?
I've taken a lookt at the aural interface in voiceover on macos sonoma (see voiceover.mp4), a few observations:
- i've just entered a single character to get more results across different groups (tested on a test instance without any content), but somehow after entering the single character "n" the announcement providing the number of results and the instructions how to navigate the list right after is being cut off somehow.
- The group titles like
content - article
,contact form
,file
and so on are unavailable in the aural interface. - The meta data of the username as well as the date and time an entity was created is unavailable in the aural interface
- The file name and the name of media item are identical but the name is only announced the first time either way (going downwards same as going upwards), the second time remains unannounced.
and unrelated to screenreaders, woult it make sense to leave a pointer either on admin/config/content/link-suggesters
or on the link suggestor edit page either in a help text or a description pointing out that for activating the link suggestor the "Entity Links" checkbox has to be ticked on a text format? At the moment that fact is implied.
re
#4
✨
Tour and No Tour available button labels should not be required
Active
you mean adding that checkbox to admin/config/user-interface/tour/settings
? and what would be the default for no tour, just "No tour"?
and in that context i also wonder since there are two opposite camps would it make sense in a follow up issue to make if the "no tour" button is shown or not configurable as well? so the site builder is able to choose if the button should be show if no tour is available or not?
ahhh ok, but those tours on the pages a workflow spans across can't be interlinked and listed programmatically for a tour? i've rechecked the shepherd usage guide https://docs.shepherdpro.com/guides/usage/ in that regard, there is no direct indication for that feature, nor the ui of the tour module is reflecting that ability. so i wonder if the following slight simplification would make sense.
Change Each tour consists of several tips that highlight elements of the user interface, guide the user through a workflow, or explain key concepts of the website.
to Each tour consists of several tips that highlight elements of the user interface, explaining key concepts and functionality on a page.
*I just left out the part about workflows and instead added the point that the tips are not solely about concepts but also about functionality available on a page.
Aside that I've noticed three more details about the points listed in the improvements section:
Change Integrated Tour UI to have a visual interface for creating custom tours
to Integrated the Tour UI module to provide a visual interface for creating new and editing existing tours
*"Tour UI" might not be necessarily clear that it refers to the Tours UI module. And aside creating tours there is also the option to edit existing tours which was not possible before either.
Change Almost 4x as many default Tours
to Almost four times as many tours available
*in regards of readability 4x
is hard to skim and process. numbers smaller ten should always be written out plus the x makes the reader also think for a second what it means in that context - instead just write things out. and why not just use "available" instead of "default" and i guess tours should be lower case since it is not a title?
Change Tour block to allow tours to run without the toolbar
to The button block for starting a tour to allow to run tours without the toolbar
...
*not necessary clear what a tour block entails. Simply adding the detail that the block contains a button to start a tour.
and in regards of the added outline color i am not sure if that isn't breaking the visual style too much.
i've tried something else. i've simply tried:
.shepherd-enabled.shepherd-element {
border: var(--card-border-size) solid var(--card-border-color);
border-radius: var(--card-border-radius-size);
box-shadow: var(--card-box-shadow);
}
which is basically the css used for cards. the grey is still too light. but i guess by using a grey border and combine it with some box shadow that should work. against the grey shepherd background it simply vanishes which is a good thing while it still provides a way to distinguish between the tour tip and the highlighted element. according tot the drupal design system the card border is using --color-gray-100 (Light Diamond - Layer border)
but as i said with the color choices used for cards the border is too light right now on the screenshot.
looks good now. went through all steps outlined before and i've also cross checked the settings page for the tour module saving the settings i havent checked before. looks all good , no errors and every saving of the form state has still a status message. thanks!
yeah no rush but as i've mentioned on the thread on slack. just comment out line 286-291 and then try to save the form. you still get status message about a tip being created or being update. so the messages are in place also without that if/else statement, plus they are more specific because they also contain the title of the tour tip that is being added or updated. i am not against those status message in any way, that feedback is important. but that if/else is adding some additional and redundant messages before anything happened is my point.
The general direction looks fab thank you! only a few nitpicks:
Each tour consists of several tips that highlight elements of the user interface, guide the user through a workflow, or explain key concepts of the website.
one question, does guiding through a workflow mean i am able to create tours that span across several pages? meaning for example a tour for adding a field? based on the answer i might come up with a suggestion for this sentence then.
Settings form to control text
the following improvement listed is not clear what it actually refers to. instead i would suggest something like:
Settings form to control the label of the button for starting a tour
And i would remove tours from the Accessibility ecosystem. the accessibility project is about testing accessibility not being the umbrella for a11y modules in general. i would still add the tour module to the accessibility category instead. cuz if you search for the term tour within the accessibility category you only find the editoria11y checker as the sole result: https://www.drupal.org/project/project_module?f%5B0%5D=&f%5B1%5D=&f%5B2%... →
no, as i've illustrated in the video. i just made a quick change:
if (!empty($input_values) && array_key_exists('new', $input_values)) {
$this->messenger()->addMessage($this->t('The tour tips has been updated. UPDATED'));
}
else {
$this->messenger()->addMessage($this->t('The tour tips has been created.'));
}
i just added an UPDATED to the returned string for the if clause. and as you can see the status message shows on the add tip form BEFORE the form is saved. plus it would have to be messaage form the else statement that the tip has been created. but still the messages are shown before the form was saved.
quickly recorded a video illustrating the current behavior
but what is the purpose and benefit of these status messages when the user arrives on the add tour tip page or an existing tour tip page, where no addition of any content, nor any edit of any content, nor any save of the form (add/edit form) has happened yet?
I can confirm that the error is gone. i am able to add a tip now. and based on your changes here https://git.drupalcode.org/project/tour/-/merge_requests/49/diffs?commit... it validates my assumption that the error was caused by an empty label when a tip is being added.
But being able to actually test the behavior now i wonder if
if (!empty($input_values) && array_key_exists('new', $input_values)) {
$this->messenger()->addMessage($this->t('The tour tips has been updated.'));
}
else {
$this->messenger()->addMessage($this->t('The tour tips has been created.'));
}
is necessary at all. at the moment when i add a new tour tip i get "The tour tips has been updated" as the status message and i see an empty to be populated "add tip" form. strictly speaking nothing happened here yet, neither an update nor the addition of a tip. on the other hand when i edit an existing tour tip i don't get a status message at all. i would expected to get the other message of the two in the if/else condition. so probably the conditions in the if clause are not correct? but i still wonder if that if/else condition is necessary at all in the first place since the status is more confusing and ambiguous than actually helping?
and on a side note "tip" should be in the singular form not plural otherwise it would have to be "have" instead of "has"?
looks like it was fixed. nice!
yes i am sure. and Interesting i think i figured out why i havent seen a focus outline. i've closed the tour tip by the mouse. take a look at the brief video. first just navigated by the keyboard in the second try i used the mouse for closing the tour tip.
@charles belov, i am on macos as well, and i also have set safari to always show the scrollbar. but i've just tested with the two other options available "when scrolling" and "automatically based on mouse or trackpad", the scrollbar is shown and rather quickly fades out right after the position changed for the next tour tip. so at least for macos the user would have a certain contextual awareness. but i am not sure how other operating systems handle the showing and hiding of the scrollbar.
so getting to the question from @smustgrave i wonder if it would make sense to commit the current state of the MR and open a followup issue for the contextual information charles suggested? cuz the module not minding the operating system setting for reduce motion shouldnt be kept up much longer imho?
thanks! I've tested three scenarios:
1) I've started a tour on admin/appearance
(as admin). the focus gets back to the "take a tour of this page" button when the tour is closed.
2) When i am on a page for a node node/2
as admin and i have the "take a tour of this page" button in the admin_toolbar and the socials sidebar in olivero the focus always returns to the button in the sidebar when the tour is closed (no matter which of the two buttons was used to open the tour) .
3) when i open node/2
in a private window, open the tour by pressing the button in the social sidebar, and close the tour again the button in the social sidebar isnt regaining focus.
i'll set back the issue to needs work. point 3 should be fixed within this issue imho. point 2 is up to debate how to handle the redundant button case, but guess better in a followup?
Based on #89 ✨ Give roles a description value Needs work i've also created ✨ Create additional default role descriptions unique to the demo_umami profile Active and updated the issue summary accordingly.
In regards of #93 ✨ Give roles a description value Needs work , i've made some additional slight adjustments to the issue summary. for the linked comments i've added a reference to the points made within the comment and i've added the point about #89.3 ✨ Give roles a description value Needs work to the list of remaining tasks, that was missing so far. That way everything is covered that was raised in the ux meetings. @anybody might need to take another look if the rest of the remaining tasks looks also fine and complete.
rkoller → created an issue. See original summary → .
I've created the follow up for #88.2, and updated the issue summary accordingly linking to ✨ Create default role descriptions Active
I'll open a followup issue for #88.2 now. will update the issue summary here accordingly and link the issue as soon as the issue is created.
thanks for working on the MR. I've quickly tested and i have one observation and one general question.
You were adding the focus with that.el.style.border = '2px solid gray';
. That way your are using your own focus outline and dont use the focus outline set for the theme.with admin_toolbar as the toolbar the color contrast is too low and the outline barely visible. it doesnt mean the actual focus outline would be meeting SC2.4.7 with admin toolbar but still i would use the themes focus styling by for example using the focus()
method in javascript?
the only question i am not sure 100% about is how to handle the case when the toolbar is available for a user, the user is on a page in the frontend that has added the tour button and that page has a tour? which of the two buttons to focus? strictly speaking tour would have to remember which of the two was pressed for starting the tour and fall back to this one with the focus when the tour is exited?
argh apologies for the video, THAT shouldnt have happened!:( i've followed your suggestion and uploaded two separate files, reduced_motion.mp4 and full_motion.mp4.
for changing the file name for already uploaded files i see no option. but i'll add a warning to the comment containing the video.
and in regards of simplytestme i am not sure if it is possible. i've just checked and the tour project isnt found there. i could share a local instance with the MR applied and send the link your way tomorrow if you want to.
and another time sorry for the oversight with the video :((((
on drupal 11 things with sdc not only look odd but are actively blocking the install. if you are trying to install same page preview on drupal 11.0.1, the composer require step works but you are unable to actually install 2.1.4 because of sdc is missing.
hm odd. i have a fresh instance and core as well as tour are each based on the git repo and the feature branch is checked out plus caches cleared. so i am not sure what might be causing the problem on my end locally? or what might be different on my end?!
now it looks like the system settings are minded, see reduce_motion.mp4. Curious what @charles belov thinks about the current MR?
hm when i am trying to add a tip i am still running into a wsod ( was trying to add a tip to the appearance page). the error seems to be the same:
The website encountered an unexpected error. Try again later.
TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /var/www/html/repos/drupal/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 431 of core/lib/Drupal/Component/Utility/Html.php).
Drupal\Component\Render\FormattableMarkup::placeholderEscape() (Line: 211)
Drupal\Component\Render\FormattableMarkup::placeholderFormat() (Line: 195)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString() (Line: 54)
Drupal\Core\Messenger\Messenger->addMessage() (Line: 287)
Drupal\tour\Form\TourTipsListForm->submitForm() (Line: 185)
Drupal\tour\Form\TourTipsListForm->tipAdd()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)
that i have to admit i not completely understand what you mean?
New tour = edit form
Edit tour = edit form
Add tip = tip list form
Edit tip = tip list form.
but in general. i agree maybe dont add the return to the tour list page at this point and commit the issue when the wsod is fixed. i see your point but at the same time i dont think relying only onto the breadcrumb is the right choice either. that has flaws, in particular for keyboard users in regards of convenience but i have to think about the problem more thoroughly and compare a few more pages in the admin ui. that shouldnt delay this issue. but i am setting it back to needs work because of the error on add tip.
After some discussion which can be found at https://drupal.slack.com/archives/C01UHB4QG12/p1724856671910839 we've agreed on not to go with the state label in one of the upper corners shown in #18. This issue dates back to a time where there still was a drop button the more than one possible state. And we currently have already an area of focus that could be used in this case to also communicate the state aka the button. you have the state of a module not added yet with the button label "add and install", in the drupal core source type modules already in the file system with the "install" button and then you have the "installed" state which currently is just a span. the problem is the span is not focusable by keyboard and screenreader users and is therefore not directly accessible. current plan is to change the markup for the installed state being also a button and add the aria-disabled="true" attribute. that way the button is still focusable, the button is not getting greyed out and can still be styled as wanted PLUS screenreader users still get the note that the button is disabled.i am updating the proposed resolution reflecting plan for the installed button.
the only thing that could be considered and discussed is to add other visual cues to the button. meaning that the user not only needs to read or skim (install and installed is close) but also has additional cues like color and icons in addition to that? so all the current three states, "add and install", "install" and "installed", are clearly distinguishable
all good, thanks for the clarification! the only detail perhaps relevant within the scope of this issue is the problem outlined in #10 ✨ GUI install multiple modules at once. Active , or is it only caused by the ui and isn't caused in the plumbing?
p.s. even if i run drush si
another time i am still getting re directed to the install page
ha it looks like it is reproducible. I have set up a new instance with the MR applied:
- i've queued token and pathauto
- then processed the queue.
- after a while processing the the spinner disappears again (by the way it would be good to have the spinner inline and in context of the process queue button, at the moment it is just an overlay across module cards, which has a high cognitive load)
- after that i simply changed from 12 to 48 modules shown -> the indefinite spinner starts again and on reload the drupal installer is shown again.
thank you! a few initial thoughts testing the current state.
- on the first look it feels odd to have "Queue" as the only available option / button label. if i want to simply install a single module i have to still queue a module and then process the queue with just a single module in. i am unable to directly install that module with one click.
Queue
andDeque
are quite abstract terms micro copy wise, same for the button label "Process queue". Plus "Process queue" sort of hides the fact that modules are being installed.- when you queue a module the process queue button gets appended at the end of the result list. problem here is for one for screenreader users the appearence of that button isnt announced (same as the state change of the toggled button from queued to dequeued) but it is also sort of cumbersome for keyboard users to have the process queue button not in close proximity where they just queued another module. if you queue one of the modules at the top of your list and you have for example 48 modules shown per page it is quite the stretch to reach the process queue button at the end of the result list by just tabbing.
oh and one fun fact. i've tried to process a queue but that had no effect (the button label jumed back to process queue after after a while). i then had one module queued then switched the number of modules shown per page count. that made the list of results reload. but the spinner spun indefinitely before any new results showed. so after while i simply did a reload and now i am presented with the installer and i am forwarded to core/install.php
:D that was completely unexpected. not sure if there is a way to provide any infos what might have caused that behavior? i dont want to just run a siteinstall and wipe the database (it seems to be it is still available and in place), wanted to wait on a feedback first. without the gui changes in front of me right now i am unable to ideate on potential other approaches ui wise for now.
and carrying over from last week:
#2118081: Unmet module requirements are too hard to find on admin/modules →
✨
Add a '#selectall' option for checkboxes element
Active
✨
Expose triggering update of media metadata + thumbnail to end users
Needs work
Wanted to test something before i answer to your point. But i ran into an error which only happens with the feature branch checked out. on 2.0.x i am able to add a new text tip, while i am on the feature branch and on the local item for tips and add a new text tip i run into:
The website encountered an unexpected error. Try again later.
TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /var/www/html/repos/drupal/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 431 of core/lib/Drupal/Component/Utility/Html.php).
Drupal\Component\Render\FormattableMarkup::placeholderEscape() (Line: 211)
Drupal\Component\Render\FormattableMarkup::placeholderFormat() (Line: 195)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString() (Line: 54)
Drupal\Core\Messenger\Messenger->addMessage() (Line: 287)
Drupal\tour\Form\TourTipsListForm->submitForm() (Line: 185)
Drupal\tour\Form\TourTipsListForm->tipAdd()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult() (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult() (Line: 80)
Drupal\workspaces\Controller\WorkspacesHtmlEntityFormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)
ahhh it bit me again that i've assumed that changes would be persistent across tabs/local items and then saved on pressing the save button across tabs. that assumption slips in from time to time. so yeah you are totally right making a redirect would be the wrong call. but it still leaves the problem that there is no direct way in the form to go back one step to the tour list view. you have to use the breadcrumbs at the moment or the navigation. but it is still advisable to have another option to move forward. that is a pattern we tried to follow in the ux meetings. one option might be having a second button (or better a third button aside save and delete) like used on the add term page: admin/structure/taxonomy/manage/tags/add
the other changes introducing the local items look great!
on d.o there are separate paths for the two:
https://www.drupal.org/project/project_module →
https://www.drupal.org/project/project_theme →
so not sure if the json api endpoint entails modules and themes or just modules. i've assumed the latter. and i thought the introduction of themes is planned at a later point? and i've just checked and simply searched for "adminimal" in the drupal.org json:api source type in project browser and there is no match for https://www.drupal.org/project/adminimal_theme → in the search results.
hm but by entirely turning off the scrolling the information is getting lost. if i remember correctly from the other issue you'Ve closed as duplicated you've mentioned that shepherd doesn't have any css animations? is that scrolling entirely javascript based then? is that the reason when the scrollTo is omitted that the focus doesn't just jump? would it be possible to have a progressive enhancement, meaning it just jumps without animation and if the operating system has no preference for the reduced motion media query (@media (prefers-reduced-motion: no-preference)
), the animation related parts would become active?
looks good, thank you! while testing i've noticed that there is one related detail i forgot. up to you if it should be fixed within this issue or shall we open a new one? if you take a look at the cancel button the target size is too small. the icon itself has 20px by 20 px in the css. for SC2.5.8 the minimum is at 24x24 pixels. for 2.5.5.(AAA) it would require at least 44x44 pixels.
hm in MR48 if "reduce motion" is active on the operating system level (macOS on my end) the scrollTo event is set to false (if i read it correctly). problem with this as illustrated in the video, in some cases the highlighted component the tour tip is referring to is outside the viewport, in one case the highlighted component AND the tour tip is outside the viewport.
true. not many do it. the only two examples that i know would be:
admin/structure/views
admin/appearance
but on the other hand also not many modules have two or more similarly named menu items... the navigation module does that, media and admin toolbar. that was the thought behind to keep the associated settings pages contained and easier to process to the user.
attendees last friday were. @amazingrando, @rkoller, @ofershaal, @simohell, and @zetragraphplug a sixth new person. but i forgot to take a screenshot at the beginning. so we would have to cross check the recording. and i guess i would have to revisit the recording checking a few details for the summary before i post on the issue we've discussed. (was a bit much all at the same time).
and in regards of the microcopy how about the following suggestions:
Configure tour settings -> Configure button label
*the h1 has already set the overall context that the page is about tour and its settings. i would make the title of this field set about, what the field set is actually about. So maybe simply go with "Configure button label"?
Tour available text -> Tour available
*i would strike the word text, plus i would rather clarify that the field is about the button label within its description
Tour text of page for which tour available -> Button label for pages with a tour available
Tour not available text -> No tour available
*i would strike the word text, plus i would rather clarify that the field is about the button label within its description
Tour text of page for which tour not available -> Button label for pages with no tour available
Two thoughts. Would it make sense to add a local items so you have one item for the list of tours and one local item for the newly introduced settings page? and the other point would it make sense to add an option to reset the two button labels back to the default?
regarding #2 📌 Rename the drupal.org (json:api) source type Active .1 i disagree on that one. at the moment with for example the drupal.org json:api tab active you have the results count there as well as a second time underneath the filter field set. any redundancy should be avoided. but the general even bigger problem is the search field is within the filter section so the scope of the search field is the drupal.org json:api source type just based on the hierarchy in the DOM as well as the general proximity. but the search query gets applied to all the other active source types as well, each of them has a result as well. to make sense, the search field would have to be placed above the source type tabs. but what is the purpose of searching across modules, recipes (at the moment locally but sooner or later also on d.o) and drupal core(which is also local). that is simply too much complexity and a potential information overload. a clear separation of concerns is probably preferable. the proposed resolution in 🐛 Filter (categories) selection is leaking over into other sources Active suggested to keep the scope of a search to a single source type with each of the sources having their own filter and dropping the results count form the tabs.
regarding #2 📌 Rename the drupal.org (json:api) source type Active .2 not sure if the ui would be overloaded with another description here or popup button. i think a more elegant solution would be to create a tour for project browser (for the search page and the details page) in https://www.drupal.org/project/issues/tour → . something i've suggested over in the discussions in the #tour chanel on the drupal slack. the development speeded up after it got moved into contrib and it is now considered by two tracks (seo and a11y) to be proposed to being added to starshot/drupalcms.
hm i consider it an editorial problem. the new field would need proper instructions in the description for the field underneath. but well as i've labeled it, the new field is more or less the alt text for the page and highlighted ui component. it is the same as adding a proper alt text or not to an image. most people make it decorational, omitting it completely or add just something brief and cryptic not helping a screenreader user understanding what an image is actually about. that is also nothing that could be fixed from the tour module end. i guess the only thing that could be done is provide clear instructions to the field, and lead by example by providing good descriptive microcopy for the new field for the core tour tips , but the field should be left empty by default.
the error is fixed (tested before and after) and i really like to dynamically show the position select field based on if a selector was entered or not. nice idea. +1 rtbc
#117.2 got fixed after pulling the latest changes. looks good!
#117.1 hmm i have given the access tour permission to anonymous, authenticated, content editor and administrator. and the route for the test tour is
entity.node.canonical
- node:2
and i have added the tour button in the header section of the block layout page.that is my setting. now:
Wouldn't that be correct? Purpose of adding the block is so it can be blocked outside the toolbar. If anonymous doesn't have access to the toolbar they shouldn't have access to the content there.
but isnt the issue title Allow Tours to be taken by users that cannot access the Toolbar (e.g. anonymous users)
so my expectation would be that me as an anonymous user who has the permission to access a tour and not the toolbar would also be able to start a tour by clicking the take a tour of this page button? but the results are:
anonymous:
clicking the take a tour button on node 4 in olivero -> no effect
clicking the take a tour button on node 2 in olivero -> no effect (even though there is a tour available)
admin (with admin_toolbar shown)
clicking the take a tour button on node 4 in olivero not the admin_toolbar-> no effect
clicking the take a tour button on node 4 in olivero not the admin_toolbar -> the tour is shown. position for the tip is right and the tip gets somehow hidden and inaccessible underneath the admin_toolbar in part. mouse users would be unable to access the close button
ok even though the outline got added to the highlighted ui component, it already gives an idea how 3px solid black would look like for the tour tip dialog modal and i would say that it is visually too strong and breaking the overall look and feel. guess solid black also with less pixels might be too strong? so maybe we should try with the drop shadow used on the cards component in the drupal design system instead? it might have too less color contrast and might need further work but that might be moved to a follow up where we could ask for example @ckrina? but the current MR applied looks like that:
thank you! looks good to me. and this issue is a good reminder to revisit the issue making the h1s and page title naming patterns consistent across the entire admin ui.
i'll get in a few more opinions. i will ask over in the group participating in the a11y track on starshot what their thoughts are. but i agree moving the counter might look. or it depends. if you move the next button to the corner in the bottom right and the previous corner in the bottom left then it would look visually even again. but with the downside that both buttons are not in close proximity anymore for mouse users. anyway gotta ask folks and will summarize their thoughts (or they comment on their own here - already "advertised" the a11y meta ;) in the discussion after our last meeting)
Opened a followup: ✨ Add a enabled and disabled section to the tour list page Active
updated the image in the issue summary per the suggestion in #77 🐛 Inform users that the maintenance mode message is not displayed on cached pages Needs review
honestly speaking the entire drupal core source type could be questioned , which also leads into a discussion i've tried to suggest and initiate several times on the drupal slack bringing together folks from the usability meeting, the recipes group , and project browser and whoever is interested to join. we've repeatedly touched the extend page in 📌 [PP-3] Figure out what to do with the install/uninstall modules page Postponed in the usability meetings and i guess the entire page has to be tackled and discussed holistically. a few personal thoughts.
at the moment on the extend page you have the local items list (list of modules), browse, update and uninstall (uninstall is referring to the uninstall of modules). all the entries shown on the list and uninstall modules page are modules already contained in the file system added by composer require.
within browse there is currently drupal.org json:api, recipes, and drupal core. none of the modules listed in the drupal.org json:api source type is contained in the file system yet, recipes in contrast currently only contains recipes shipping with drupal core which are already contained in the file system BUT sooner or later it will also list recipes on d.o not contained in the file system yet, while drupal core strictly speaking simply mirrors a subset of the modules on the list page, but all of them already contained in the file system.
personally i would vote for a clear separation of concern. the sources shown under browse should only list extensions (modules, recipes, themes) that are NOT in the file system yet and remote. while all the local items aside browse on the extend page have the entities listed on their tabs be already added and available in the file system, either installed or uninstalled. and i would have one local item for modules, one for recipes and one for themes (and the top level item appearance could be deprecated - something you dont use on a daily basis which would be the sole reason to justify having a top level item - something i've realized in one of the drupalism meetings where @cellear made that valid point). but that way concepts would be structured more clearly with a separation of concern. at the moment everything is overlapping and potentially confusing if you dont know about the underlaying concepts and their constraints. just a rough outline and idea but the longer i think about it the more sold i am on it.
but yeah out of the scope for this issue but i really think it would make sense to bring several groups together and having a discussion. cuz in particular if you test the prototype for starshot the problem is further emphasized and amplified. you have an overwhelming modules list page and no direct touchpoint with the list of available recipes nor any way to directly see which of theses recipes got applied setting up starshot /drupal cms. the user is in complete lack of situation awareness.
the needs another rebase. after adding when i try to checkout the branch i run into the following error:
$> git checkout -b '3317769-cke5-entity-link-suggestions-11.x' --track drupal-3317769/'3317769-cke5-entity-link-suggestions-11.x'
error: The following untracked working tree files would be overwritten by checkout:
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Connection.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Install/Tasks.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Connection.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Install/Tasks.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Connection.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Install/Tasks.php
Please move or remove them before you switch branches.
Aborting
it happens when the issue fork was created or last rebased before this issue went in https://git.drupalcode.org/project/drupal/-/commit/8b368d712d83900765744 on the 13th of august fixing spelling errors. the switch to camel case is the problem where the none case sensite file systems ( i am on a mac for example) in combination with git struggles and leads into this error.
oh i now see and understand what you are referring to. so under the top level menu item content
the order is "edit [entity type] [entity name]" that is what you were referring to, while under the top level menu item structure
it is the other way around "Edit [entity name] [entity type]" which is what i was referring to. the latter, front loading, is in the context of readability the preferable and recommended choice. a few months ago we've ping ponged about that very topic between the ux and a11y meetings. a summary and the outcome of those discussions can be found in this followup issue that picked up some of the outcomes:
https://www.drupal.org/project/drupal/issues/3370946#comment-15203381
🐛
Page title should contextualize the local navigation
Needs work
and i've also created a google doc comparing all h1s and page titles across drupal admin ui https://docs.google.com/spreadsheets/d/1XhQtXZA1JCwa8qnE3vp8gyW8HJT8zZxg... . and that front loading pattern is something i have strong opinions on. was a good reminder to revisit the following follow up issue
📌
Refine the page title if possible
Active
.
Cuz the problem i have with the "edit tour custom block edit" example. you start with "edit" then you get way more "general" and then you get more specific again with the actual title. the context, that you are within a tour, you already know before, since you got onto that edit page from the tour list page.
while the other way around, first you know you are on an edit
page then you know what document you are editing, the "custom block edit" page and then you get to know the context that it is a tour. screenreader users as well as sighted users can jump off at any time with this pattern. screenreader users skim with their ears sighted users with their eyes. with "edit tour custom block edit" you first have to scan edit (the action) then tour (the context) and then finally starts part of interest. while with "edit custom block edit tour" you start with edit then custom .... maybe that is already enough, the person knows they are in custom block edit... while if they are uncertain about the context they can still go on for reassurance that they are within a tour. (in case there are several open tabs for example or they have a small working memory).
both approaches, the one you suggested and the one i suggested would be sort of consistent with the rest of drupal at this point, but personally i hope it gets unified on the front loaded approach sooner or later.
i've checked alpha13 of shepherd 12.0.0. ( i guess that is the latest). the dialog element/aria-modal issue for upstream is still necessary. shepherd-element.svelte uses a div and doesnt have the aria-modal attribute either ( i am only on the feedback from mike, just asked him as a reality check before i create the issue upstream). and on the button in shepherd-button.svelt i also dont see aria-pressed nor aria-haspopup. so probably it would make sense to open an issue upstream for that as well. guess both should happen in combination.
hmmm a screenreader once told me "ideally" the content for sighted and screenreader users should be identical. and sometimes you have "semi"sighted folks who are able to grasp visual cue but rely on screenreaders. having the step count in the title and in the bottom right corner would be sort of redundant.
my other thought was make the steps in the title visually hidden for sighted users while make the count in the bottom right corner hidden from screen readers. but that idea would collide with keep the content in line for sighted and screenreader users if possible.
yeah i also doubt that at least the designs for the tour module with those dark backgrounds are up to date nor reflecting the current design styles. i was more drawn towards the style which is used for the cards. but that had the downside of a too low color contrast for the outline. so maybe you could add the one-liner you suggested with outline: 3px solid black
just to see that change in context?
looks good. and completely agree on moving the disabled section part to a follow up.
and just to understand things correctly, for the ability to have a filter it is necessary that the page is an actual view? but yeah with more than 20 or 50 tours a filter would be helpful or lets say better mandatory.
oh and removing the two columns made the page less overwhelming. i like that. but i wonder with your example of 100s of tours it might be helpful to have a description column so the distinguishing is not just based on the tour name (which might be also cryptic in case you have 100s of tours. i doubt you will be able to pick clear and easy to grasp tour name each and every time).
now the order looks off:
instead of
$form['#title'] = $this->t('Edit tour <em>%label</em>', ['%label' => $this->entity->label()]);
better
$form['#title'] = $this->t('Edit <em>%label</em> tour', ['%label' => $this->entity->label()]);
ah you already updated the issue summary, thanks! and one thought i had comparing the tour with for example views, would it be useful to also add a description field to a tour? for the sake of consistency it would make sense but probably also it could be helpful to provide the gist what a tour is about?
the em tags are getting rendered.
That one slipped through, apologies. I've taken a look, a few observations and thoughts:
i would rename the title of machine_name
in the tabel header to Machine name
, that it is consistent with other tables like for example admin/structure/views
I would also make routes
upper case so it is Routes
Then all the tours that the tour module is shipping with have no module dependencies, that way you have one empty column. But i wonder in combination with Routes. Are those two columns necessary at all? does someone really need those two details in the tour overview page? are dependencies or routes are compared on the overview page between different tours? is there a need for that? otherwise those two informations might be also good enough only be shown on the edit page of a tour maybe?
and in regards of "Apply the same pattern used on /admin/structure/views separating enabled and disabled tours." you wrote "if there is a recommended way to turn config into views ", but on closer look is it really necessary to turn this tour page into a view? as far as i understand admin/structure/views
is not a view either? that pattern is only about having two tables instead of a single one. one table for enabled tours and one for disabled. so maybe that way it is not necessary to turn that tours list page into a view? cuz there is also no need to sort based on a particular column or leave the user the option to alter the columns and so on.
I'll change the issue back to needs work for the first two points about the micro copy.
uhh damn when i reread the issue summary after i saw the notification that you assigned yourself i realized i'Ve created a dedicated issue 🐛 The page title "edit tour" is too generic Needs review for point 1 in the proposed resolution. there were too many issues i've created and i completely lost track and forgot that about the linked issues. :( that was not intended.
Looks good! the only minor nitpick i've just noticed. would it make sense to wrap the tour name in an em tag in the h1? so it is in line with the pattern used on for example content types (admin/structure/types/manage/article), block types (admin/structure/block-content/manage/basic) and so on?
oh the button markup is generated by sheperd? i've assumed that would be on the tour module and or drupal end? will take a look if i am able to figure it out if sheperd 13 which is the current version i suppose is supporting it.
3px solid black would be more than enough. i worry that it might be even too strong visually.
i've taken a look at the drupal design system at: https://www.figma.com/design/OqWgzAluHtsOd5uwm1lubFeH/💧-Drupal-Design-system?node-id=10154-0&t=VmN7KSHpguxzhIGq-0
maybe orient to the styling of a card in general? well there is also a dedicated section for the tour but that is a work in progress. and i am not sure how the black modal will work with the dark grey background.
I've had already added it to our shortlist on 📌 Drupal Usability Meeting 2024-08-23 Active . But MR3037 needs a rebase, on a none case sensitive file system ( i am on a mac) i run into the following error:
$> git checkout -b '2983456-expose-triggering-update' --track drupal-2983456/'2983456-expose-triggering-update'
error: The following untracked working tree files would be overwritten by checkout:
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Connection.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysql/Install/Tasks.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Connection.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestMysqlDeprecatedVersion/Install/Tasks.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Connection.php
core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestPgsql/Install/Tasks.php
Please move or remove them before you switch branches.
Aborting
it happens when the issue fork was created or last rebased before this issue went in https://git.drupalcode.org/project/drupal/-/commit/8b368d712d83900765744 on the 13th of august fixing spelling errors. the switch to camel case is the problem where the none case sensite file system in combination with git struggles and leads into this error.
Added a remaining tasks section with the child issues listed in the sidebar. #23 🌱 [META] Improve accessibility of tour module Active has still to be rechecked and reevaluated another round if there are more child issues to be created