πŸ‡ΊπŸ‡ΈUnited States @bnjmnm

Ann Arbor, MI
Account created on 3 November 2012, over 11 years ago
  • Staff Software Engineer at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
  1. There was a suggestion for "When the Menu selection changes, move the focus to the Parent link select list." - this isn't something I'd approve in my role as accessibility maintainer. The expectation is focus would remain on the control being manipulated. Deviating from this expectation would likely cause more confusion than it would offer any benefits.
  2. The now-two select elements should be semantically grouped, you can reference the related elements section of these W3 docs.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

+1 to the points brought up in #10.

Not using px prevents a11y breaks. I confirmed the current MR still allows resizing fonts via browser settings and custon stylesheets. These were the two accessibility risks that came to mind and neither appear to be a problem.

I also agree that issues such as the one in Bootstrap are the responsibility of the theme, but I highly recommend offering an MR to popular themes that resize the nav text to unusable sizes. Why require several theme maintainers to figure out a problem that several people in this issue alone would already know how to address.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Re #10
I haven't looked at this for a bit, but I recall running into multiple issues when there were DOM changes to the form. This could be mitigated by adding the progress throbber outside of the form, perhaps something like a bar at the top or bottom of the viewport. If we went that route, it may be simplest to not bother with progress messages in this issue's scope as I could see that opening a design rabbit hole that would probably be worthwhile, but not worth blocking some incremental progress.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

It is very possible some of the MR feedback is for my own code from the first JSX demo that has been ported to the next demo.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Added feedback to the MR. It is all trivial other than one thing, and even the non-trivial one shouldn't be too demanding. Overall looks good and looking forward to rtbcing.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Does this seem better and attracts enough attention to the note?

It's better than not having the association, but it's not great. I think updating the descriptions has a better result as it keeps the UI clean, removes the edge case, and the relevant information is in the correct context. Also less code needed πŸ™‚

Something like this looks better and I think has a higher chance of benefitting a user vs. the current tacked on message.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I've largely addressed the JS needs that this issue was assigned to me for, and there are added FunctionalJavascript tests for that functionality. There's one last @todo in the JS I need some clarification on and will ask @wim.leers about that.

The big JS stuff that made it useful to assign the issue to me is bascially done, though. Unassigning myself but leaving at NW since there are still todos in .php and .yml to address before this can switch to "Needs Review".

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I think it's reasonable to complete this issue with the intent to complete πŸ“Œ Move the first two steps of field creation into a modal. Needs work before the next minor release, otherwise we revert. Doing this successfully will demonstrate that we can be a bit more incremental than our current cadence, and my hope is that can increase innovation momentum in Drupal. If the followup doesn't make it in it will demonstrate that such an approach is unwise to do in the future, so lets be sure to avoid that and show we can handle a bit more flexibility.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Lets get rid of the info specific to decade-old versions of PHPStorm, and remove the platform-specific keyboard shortcuts in favor of steps that work the same regardless of platform.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Nothing mentioned in the issue indicates this is specific to the ckeditor5_dev module. Does this issue only happen if the ckeditor5_dev is enabled?

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

It still dictates the links and the content inside, is it still an accessibility issue ?

Afraid so, just because a screenreader can see it does not mean it's correctly categorized/conveyed. Two issues is that they are contained in a <form> with no form elements, and they are not logicially grouped - they're just seen as individual links where it's not immediately clear that they represent a single choice within a group of options.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I'm not sure about adding info to the bottom of this form specific to choosing media. The options themselves already include description text specific to that option - and option-specific descriptions should live there. Ideally the current media description text could be shortened (its longer than it needs to be IMO) to make room for this info. Having it as a footnote technically addresses the regression, but I don't think it actually improves the experience.

If rewriting that description is not a feasible option, at the very least associate the bottom-of-the-form text with the media option with an asterisk. In its current state I think it is unlikely a user will even read it - they will see the option they want then click it to proceed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

So the changes here turn the radio elements into links. In order to make that change accessible it would be necessary to:

  • Remove the <form> element. By changing these to links, it is now semantically incorrect as it is a form with no form elements (but in issue #3386762, these would open modals and could be buttons instead... and could still live inside a form)
  • The links need to be styled as links. If clicking something takes you to a new page, the appearance should be consistent with the other elements that would take the user to another page. (but....... in issue #3386762, these would open modals and could be buttons and retain their styling)

The changes needed to make the changes in THIS issue accessible would effectively be unnecessary once #3386762 lands. Those same changes would also make #3386762 more difficult to land as it would require undoing much of that work to get back to where HEAD is. I can think of three possible approaches to navigating this:

  • Include modal support in this issue instead of #3386762. However, I think that was the originally intended approach and there were objections to it so I'm not sure how feasible that is.
  • Make the changes listed above to make THIS issue accessible, and accept that it will add complexity to #3386762 (open in modals)
  • Approach this issue as if #3386762 will land (make the options buttons within a fieldset), but commit to reverting the changes here before the next release if it turns out #3386762 can't make it in. I'm not opposed to this approach... but I'm concerned that there's no formal policy in place to ensure the revert happens. What is here is acceptable as an intermediate step, but is not acceptable to release. (maybe there's a way to only commit to 11.x and disallow addition to 10.x until #3386762 is there as well?)
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The default branch for new work is 11.x. Anything done on 11.x also gets added to the next 10.x release unless specifically designated for 11.x.

Additional feedback is in the MR that should be addressed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

If there's evidence this could benefit users without introducing problems, I'm happy to FEFM approve form-radios getting added in a manner similar to other form elements.

Just make a followup for a potential "do it right" version #19. I think it's a noble pursuit, but even if it were to happen, it would not be quick (FE this issue is almost 6 years old). While they wait, I'm OK with letting folks enjoy the consistent-but-not-DRUPALSTRONG addition of form-radios .

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Calling out an edge case where this code is problematic: when a Drupal media item is the selection that the link is targeting, the ranges here are not populated, and the data- attributes are not added to the link.

Glad this was mentioned while I had some time to work on this. I have this largely working locally and will push once I've addressed some details. This particular issue is much easier to address when all of the functionality is provided by core plugins, so an especially good call on core-ifying this linkit feature.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Thanks all for confirming this is no longer needed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

@shweta__sharma Perhaps I'm misreading your images, but the issue specifies this is related the up-pointing chevron, and both screenshots you provided are examining the down-pointing one.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Pretty straightforward change. Merged!

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Re my comment in #17 - there needs to be an assessment of common modals used in Claro and determine if the current solution provides adequate space for the modal title to be understandable/useful.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I opened MR 6112 as patches don't seem to be testing properly. Most of the 1800+ test failures in #55 and #58 do not seem to have anything to do with the code changes. Among other things there are "missing composer.lock" messages, and that file is not touched in either patch. The MR I created is failing ~3 tests, so those will need to be addressed, but that should be straightforward compared to the unexpectedly huge fail count that suddenly appeared (and not at all the fault of @Prashant.c or @Hardik_Patel_12)

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

#14

I tried to figure out where the CSS for the ellpsis in Stark was coming from in order to compare the style changes, and now I'm confused:

jQuery UI's CSS in the vendor dir (πŸ‘ on including your grep!)

#11

Based on @Utkarsh_33's testing, it looks like Olivero and Stark instead truncate the word with an ellipsis, rather than wrapping it (probably text-overflow: ellipsis;?). We should do the same here.

I probably agree with this, but since Claro uses larger fonts I think it's worth checking some common modals at narrow-but-common widths. Fewer characters can fit in that titlebar with Claro, so I'd like to rule out the truncation making it unusable. For example, things like deleting content via admin/content are done via Modal and those could get long. It's also possible that some translations result in significantly more characters. This may have been the reason Claro opted to override Jquery's default dialog styling and allow multple lines.

If the conclusion is to match the other themes, then the feedback in the MR I left should be addressed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

assertWaitOnAjaxRequest() now fails if during the wait window.drupalCumulativeXhrCount never exceeds 0, so any uses of assertWaitOnAjaxRequest() that weren't actually needed will now fail.

So for tests that were previously passing and now fail on assertWaitOnAjaxRequest(), the first thing to try is removing the call entirely. It was very common to add these calls in places where they were not needed, where they were used for waiting on a JS operation to complete that wasn't necessarily something that used the Drupal AJAX API.

It is possible removing assertWaitOnAjaxRequest() will result a different test failure. Calling assertWaitOnAjaxRequest() when no Ajax calls are present still added a 10000ms wait before proceeding, and tests may have depended on that wait. Ideally, if the assertWaitOnAjaxRequest() removal results in a fail, it should be replaced with a wait for the specific expected change.

One of the reasons for this change was due to every unnecessary assertWaitOnAjaxRequest() adding 10000ms per-call to each test run. The more of these removed, the faster you can expect your tests to run.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Spotted a few things in the MR I'd like updated, but hopefully nothing major. Overall this looks like a good addition by adding CSS support to the prettier functionality we're already accustomed to with our JS files, and the formatting changes are not too jarring.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

It . I'm guessing this may be 10.2 conflicts that impact the entire module

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

AFAICT the cache invalidation logic in ::enable() is not correct nor necessary? πŸ˜…

I could totally be missing something though. πŸ˜‡ But in that case, docs are needed to explain why this additional cache invalidation logic is needed. πŸ™

Attached is a patch with the test that fails if I don't do that cache clear in enable(). I couldn't find a reliable tag to invalidate. When a text format was disabled, the entity would no longer have the corresponding config:filter.format.the_text_format tag. The scorched earth cache bin blowup was how I could get it to work, but I'd prefer a more targeted solution as well. Clearly there is data in the DB that is aware of formerly-used text formats, and perhaps that is the way to do it without annihilating entire bins?

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Currently, visibility can only be toggled by counter device, so it is not possible to test screenreader/AT implementation.
I left feedback in the MR regarding how to address this, and once that is completed I can potentially provide accessibility signoff.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Button purpose is clearer, and element being debugged remains highlighted until there's a click.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Created form-specific contexts and app-specific contexts and templates for inputs and textareas are wrapped in a HOC that updates/subscribes to the central form state

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This is reported as a regression caused by updating Stylelint to 15, which was committed on July 17, 2023.

I spot-checked the first three files in the merge request, and they are change code that was in place prior to the Stylelint changes on July 17, 2023, so it looks like this MR is making changes and enforcing rules that were enforced by any Stylelint config in core. Some of these changes look nice, but they'd need to be proposed as changes to the configured standard, as they do not appear to be regressions.

Also worth noting, in Stylelint's "Significant changes" page for version 15 it points out

The deprecated rules will still work in this release (with a deprecation warning). In preparation for the next major release, when we'll remove the rules from Stylelint, we suggest:

. So while it is true these rules are deprecated and we should prepare for their removal, they should currently work as they did in earlier versions.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

JSX Debug console output has richer info and more details.

JSX and Twig debug now have template suggestions, too.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This updates _jsx_get_variable to unpack referenced entities and (matching twig behavior) single-value fields 'value' property does not need to be accessed within an array.

This gets JS working again on the Umami home page so I want to get this in, but while working on it I also noticed (probably later than I should have) that _jsx_get_variable and the jsx_devel content debug functionality have a good amount of overlap with some possible inconsistencies (FE the debug shows the key as #media, but the variable outputs it as media). That will be for another commit.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Fingers crossed this stops the commit wackamole I inadvertently started this AM 😎.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Merged to get the theme working again πŸš—

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Addressing the checkbox issue could go in propsify (and I just filed a PR for a different issue in that repo so I'm in the flow there).

The select/option one is a little tricker - it moves the selected info from the <option> element to the parent <select> element, so it winds up being a bit more involved than renaming the props. Because of this I'm a little on the fence where the best home for that fix is.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The CKEditor 5 upstream issue β†’ looks tricky and I'm unsure how long it might take to get a fix there.

However, this is something that can be addressed on Drupal's end for at least one very common use case: when used inside tabledrag. We know enough about the structure it will be in that we can use some custom JS to set the CKEditor-containing table cell to a max-width, which will result in the editor properly implementing any needed ... and keeping the editor width within the table it belongs to.

I added a solution to the MR. It would benefit from tests, but it also seems difficult to test but maybe there's something elegant I'm overlooking.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The test I added demonstrated that different caches need to be considered when re-enabling a text format, so glad the test was there, and this has now been addressed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Thanks for the extra context @Wim Leers -

\Drupal\Tests\filter\Functional\FilterAdminTest::testDisabledFormat() already tests this?

That existing test confirmed that disabling a filter would result in the field being unavailable, but there wasn't anything for the use case of re-enabling the format in the UI and confirming it was again visible. I expanded the test in the MR to cover this scenario so everything I mentioned in #125 is now addressed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

@omkar.podey created followup πŸ“Œ Improve representation of the list of enabled and disabled text formats Active so I'm removing that tag.

I updated the issue summary, so I'm removing that tag.

Concern

I manually created content with a text format, then disabled that text format. I am still able to access the content created with that text format despite what the confirmation warnings say. I'm not surprised since it is displaying the field contents as saved in the DB - not passing it through any text format code.

πŸ‘‡ Look at this page content created with a disabled text format. It is still being displayed despite the confirmation page warning stating otherwise

This needs additional tests

The only test added testEnableAndDisableOfFilter() successfully confirms enable/disable operation links appear as expected, but we also need to test and confirm

  • The before/after when enabling a text format
  • What actually happens when a text format is disabled, because the current warnings don't seem to be accurate (or if they are, then the test can exist to demonstrate my manual tests are flawed)
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This functionality is now available in the jsx_devel module

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Also regarding the comment related to #195 related to (The toggle comes after the text box...) i think I'm not a 100% sure about this change.So maybe some further help is required on that point.

If a user navigates to the password field via a screenreader, and that password field is currently set to type="text", thus making the password field visible. They are not aware of this fact until after they've entered a password and navigated forward to the hide/show button. Ideally, when the password is fully visible, an AT user should be aware of this as soon as they land on the password field.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
  • The toggle uses a <button> tag, as it should. However, it is styled like a link - there should be a means of visually distinguishing the button (which triggers functionality) from a link (which takes the user to a new destination). This should be happening regardless, but it's particularly a concern here since the looks-like-a-link toggle is quite close to an actually-a-link element that is styled the same
  • This has been mentioned in other comments, but I notice the password input in the login form does not have this password-show functionality. It seems like a good idea to include that here - even if it were planned for later that would be the most effective way to confirm the implementation can support this use. If there's a solid reason for not including it within this issue's scope, that should be included in the issue summary (it is possible I missed an explanation in the many comments above)
  • The final item in #195 (The toggle comes after the text box...) is probably worth addressing, even if I haven't seen it addressed in other accessible password widgets. Any measures that can be taken to avoid unknowingly visually revealing a password should probably happen. At the risk of having a
    different take from the more-thorough #195, I'd prefer this information to be visually hidden as the hide/show state should already be reasonably apparent to sighted users via the buttons, and the password fields are already a bit visually noisy as-is. The #description approach might but would need to preserve any existing description. Another possibility is using aria-description and appending any text from the description element.
  • It seems like there may need to be some additional styling to remove the MS Edge provided hide/show controls. My usually-reliable Virtualbox that lets me use Windows 11 is not cooperating, so I'll either need to get that addressed or better yet someone with an already-available Windows environment could check that out.
  • The change record needs more detail. It's currently showing before after screenshots of the profile form and says "Previously the users were asked to re-enter the password in the confirm password field and if the user enters the password it cannot be viewed in both the password field. So with this MR we want to deprecate the confirm password field and add a button next to the password field which will enable the user to see the password." Lets update it to remove mentioning "MR" (once merged it's no longer an MR πŸ˜‰), describe the new render elements, and provide code examples of how they are used compared to their predecessors. One of the best ways to get a sense of what goes into a good change record, is to go through the list of existing change records β†’ and see how they are written.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Some scenarios come to mind that I think should be accounted for with whatever solution we come up with:

  • I do like the thought of getting a little opinionated with the goal of better UX. I'm on board with questioning "because that's how we've always done it" choices, but I also want to be considerate of existing sites that might be disrupted by the change. In particular, some sites might have additional top-level items that lead to destinations that aren't just section summaries, and removing those links could make it difficult to access something important. Possible Solution: make this a configuration option. On new sites, it will default to not having "Overview". Existing sites that upgrade to the new navigation could either automatically have "Overview" enabled - or perhaps only enable Overview if there are additional top-level items beyond what core provides.
  • In the Menu UI, each item has a field for "Link", including the top-level items. Even the current MR turning it into an Overview link messes with the the menu UI natural mapping. I think it would be even more disruptive if Link field does nothing for links in that position. Possible Solution: Solve a tricky design problem and update the menu UI to distinguish top level collapse/expand items from regular menu link items.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The nightwatch MR has basic accessibility tests running. It's good to have these running, and this demonstrates that adding Nightwatch doesn't require any major errfor.

The Axe A11y tests already caught a duplicate ID, which is now fixed in that same MR. Given that this already caught an accessibility bug, I think this basic implementation should get in sooner than later so it can surface other accessibility problems as this project evolves.

The NW testing can get more comprehensive than what is currently there if we see the need. I think beginning with something simple like this is helpful as it gets basic A11y checks going and is a starting point for additional NW tests.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Yes, I think so. That's why I'm hoping for a shorter classname, maybe something like .drupal-noreset.

I'm pro-BEM in many cases, but (as a front end framework manager / committer) I'm not opposed to approving something with a shorter classname. I think it's helpful to see at a glance if something has been un-reset and .drupal-off-canvas-wrapper-noreset is a little too long for it to register quickly.

For this to get further we'd need some kind of test coverage like what I requested in #6. If anyone following this issue is interested in doing this but unsure where to start (either writing the tests, or just getting it set up locally), find me on Drupal Slack and I can assist. This seems like a pain point worth solving sooner than later.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

It seems like there are more changes happening in the Svelte files than needed, particularly the changed breakpoints. I'm also quite open to the possibility that this is a byproduct of me not fully getting it yet πŸ™‚.

There's definitely a need to have the enclosing <ul> be aware of the grid/list view, which on HEAD is only available to the <li> elements. It looks like that is part of what the MR is doing. I was able to get that taken care of with fewer overall changes to the .svelte files. It may be best to centralize the toggleView available to <ProjectGrid> and <Project> by adding it to the store, but what I have below seemed to work really well with the CSS changes from the MR

diff --git a/sveltejs/src/ProjectBrowser.svelte b/sveltejs/src/ProjectBrowser.svelte
index b8a6d42e..4a232d2b 100644
--- a/sveltejs/src/ProjectBrowser.svelte
+++ b/sveltejs/src/ProjectBrowser.svelte
@@ -293,7 +293,7 @@
 </script>
 
 <MediaQuery query="(min-width: 1200px)" let:matches>
-  <ProjectGrid {loading} {rows} {pageIndex} {$pageSize} let:rows>
+  <ProjectGrid toggleView={!matches ? 'Grid' : toggleView} {loading} {rows} {pageIndex} {$pageSize} let:rows>
     <div slot="head">
       <Tabs {dataArray} on:tabChange={toggleRows} />
       <Search
diff --git a/sveltejs/src/ProjectGrid.svelte b/sveltejs/src/ProjectGrid.svelte
index 2ee0de85..14c57d74 100644
--- a/sveltejs/src/ProjectGrid.svelte
+++ b/sveltejs/src/ProjectGrid.svelte
@@ -21,6 +21,8 @@
     loading: Drupal.t('Loading data'),
   };
 
+  export let toggleView;
+
   $: filteredRows = rows;
   $: visibleRows = filteredRows
     ? filteredRows.slice(pageIndex, pageIndex + $pageSize)
@@ -58,7 +60,7 @@
         <div>{@html labels.empty}</div>
       {:else}
         <ul
-          class="projects-list"
+          class="projects-{toggleView.toLowerCase()}"
           id={$activeTab}
           role="tabpanel"
           tabindex="0"

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Re #37

The patch is introducing jQuery UI dialog dependent code to the Ajax system. I think we should look if we could move that to the code that integrates the jQuery UI dialog with the Ajax system in core/misc/dialog/dialog.ajax.js.

Took a look at dialog.ajax.js and noticed that what was believed to be a jQuery UI focus management problem was actually something core introduced many years ago with #2113567: Views dialogs don't close via escape. β†’ . Adding a condition to only refocus the dialog if focus has been lost seems to fix the issue reported here. Pushed an MR. Lets see if DrupalCI agrees.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Sorry but i still see no reason to merge `i` button positioning with tooltip component.

I haven't stated a position on the merging / separation of these components one way or the other so no apology needed. Any reservations expressed thus far are specific to the terminology "tooltip", which should not be used as that is not what is being created here. This is hiding/showing additional information via a dedicated disclosure button, which makes it a toggletip. A tooltip would be something that offers additional info that is triggered by interacting with an element/text on the page that has a purpose outside of being a disclosure for the tip content. Something like tipTrigger and tipContent would be more accurate labels for what I think is being attempted here.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Did you check this MR?

Admittedly I didn't get very far as I saw the addition of things labeled "tooltip" when we are decidedly going the toggletip route, not tooltip. For more on that distinction, this article explains it pretty well. Since you're apparently not adding a tooltip per that understanding of it, this can be looked into further - but if this is determined to be a good approach the naming should be something other than "tooltip".

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

@bnjmnm, yeah the patches in https://www.drupal.org/project/entity_embed/issues/2640398 ✨ Allow elements to be inline CKEditor Widgets Needs review don't work with any version of CK5. Would be great to be able to embed entities inline like we can with CK4.

The first step towards getting there would be completing the issue you referenced: ✨ Allow elements to be inline CKEditor Widgets Needs review .

This issue's scope was for ensuring that any existing (I.e. in a tagged relase) CKEditor 4 features are also supported by CKEditor 5, and that appears to be complete.

The inline embed functionality is only provided in an issue that has not been completed (#2640398). It would be reasonable to expand the scope of #2640398 to include CKEditor 5, but this issue was specific to providing CKEditor 5 support for an unpatched entity embed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I can't see problem here if we build it with 2 components still.

The problem with providing two components in this issue instead of the one proposed is

  • Scope creep
  • Adding a component that has not been vetted as accessible
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I like the idea for an additional tooltip component alongside the toggletip being built here, but it shouldn't be added within the scope of this issue for two significant reasons:

  • Before work even started on this, we confirmed there is an accessible way to implement toggletips. This isn't the case for tooltips, and in some cases the recommendation to make truly accessible tooltips was to use toggletips instead
  • Even if a fully accessible tooltip approach was discovered, it is best implemented in a separate issue. I's not necessary to introduce them at the same time, and keeping them separate means less of a review burden and avoids the scenario where one render element is fine but the issue can't land because there are concerns about the other one.

I closed the MR to avoid additional work going into something that, while a good idea, should not be happening in the scope of this issue. The branch still exists, though, and can be used to port the code to a dedicated tooltip issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This also needs an update hook for the entity defintion. I noticed this was added in some of the other Revision UI issues and wasn't sure why it was needed since updating the definition in the PHP class provided the expected functionality. The EntityDefinitionUpdateManagerInterface docs explain why, and there's are existing examples to work from and just swap in the media specific stuff.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Two remaining threads to address. The changes are straightforward enough that whoever makes them is welcome to self-RTBC this and get it back into committer consideration.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

To determine if this policy is feasible, it would be worth SDC-ifiying some core components that would benefit from better default styles. Media Library comes to mind. How does this relate to the policy feasibility?

Perhaps not the best wording. On paper the "no-BC-for-SDC" sounds good, but an MR that converts some core components to SDC might reveal additional considerations that we're not currently thinking about, and these conversions could included as part of the switch to non-experimental.

I did think of a BC consideration that should probably be in the policy. As detailed in the IS I think it's reasonable to allow SDCs to change as needed without too much BC fuss. However, for Stable 9 extending themes should have some kind of BC protection for components that have been converted to SDC - at minimum preserving the markup but ideally able to leverage the same preprocessor too.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Re #173. Perhaps I'm reading this incorrectly, but are asking for functionality that is not part of a tagged release, but only available y applying a patch from the issue ✨ Allow elements to be inline CKEditor Widgets Needs review ?

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Pushed a fork that gives us something to start with. Head to the JS console in your browser and type drupalSettings.jsx and it will return an array keyed by the templates used on the page:

Each one opens to show available properties and their types. Objects that are not content entities stop at the class name. Content entities will show its fields. This previews the content avilable in addition to the property names/types

Having it in DrupalSettings doesn't mean the JS console is the only way to view this. JS behaviors could be added that take this data and make it available in the UI or HTML comments.

Some things this doesn't do in its current state (but could)

  • This is not yet toggle-able. It definitely would need to be
  • If a template is used multiple times on the page, it still only appears once in the object. Could be confusing if the field preview is part of this.
  • Does not elegantly associate the template info with what is on the rendered page. This could likely be done by adding attributes with unique values and adding JS that queries those attribute values.
  • Probably a better way of formatting the contents so it's clearer how it corresponds to the template structure.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This solution is something I'd FEFM-approve some form of. Seems like a fairly gentle change that would address an annoying pain point with off canvas dialogs.

  • This should include tests that use getComputedStyle to confirm styles are applied/not applied accordingly
  • If we go with a long class name as suggested, they should be BEM. If the class name was shorter I'd be inclined let that BEM requirement slide. (In other words, if it's already unweildy, then BEM should be there.. but something easier to understand at a glance might be better DX than BEM.
  • Is there any benefit in having two classes - one for a single element and another for .class * so all child elements are reset immune?
  • I'd like to see a comment in the CSS explaining the use of the no-reset class(es). The ideal place would be where someone is most likely to look when they are frustrated and tracking down the CSS that is messing with their UI.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Opened MR that handles the absence of an element more gracefully. As noted in #12 - this callback is set up to be invoked when an element is selected. While it's not clear why it would run otherwise, it's easy enough to find those instances and exit early to avoid JS errors.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Workaround implemented and tests added.

Production build https://api.contrib.social 0.61.6-2-g546bc20