Comments

BarisW’s picture

Status: Active » Needs review
StatusFileSize
new647 bytes
tr’s picture

Assigned: Unassigned » tr
StatusFileSize
new10.92 KB

This module needs a LOT of changes of changes at this point. Here is a large patch that makes it work in the current (as of today) version of Drupal 8.0.x. I've tested the announce part, but I don't know how to test the tabbing manager part since that never worked for me.

tr’s picture

Category: Task » Bug report
Priority: Normal » Critical

Since the module is unusable and gives a WSOD in its current form, this qualifies as a critical priority bug.

mgifford’s picture

That patch still applies nicely. I don't know how to effectively test this module though.

wim leers’s picture

Status: Needs review » Needs work

Looks splendid! :)

Sorry for the long delay.


Here are the final bits that need to be changed before this can be committed. If you want me to commit this first, and then fix it in a follow-up, or want me to make these changes, that's fine too. Again, sorry for taking so long!

  1. +++ b/config/schema/devel_a11y.schema.yml
    @@ -0,0 +1,31 @@
    +      label: ''
    ...
    +          label: ''
    ...
    +            label: ''
    ...
    +          label: ''
    ...
    +            label: ''
    ...
    +            label: ''
    

    These labels are missing.

  2. +++ b/devel_a11y.module
    @@ -7,66 +7,23 @@
    +  if (!\Drupal::currentUser()->hasPermission('access devel information')) {
    

    Because we check this before making changes, we should also unconditionally set $attachments['#cache']['contexts'][] = 'user.permissions';

tr’s picture

Yes, I'd prefer a commit now then fix in a follow-up.

I left the labels blank because I didn't know how to describe those variables without doing more work to figure out what exactly they were supposed to do (especially with regards to the tabbing manager). Plus, the labels aren't really used anywhere so they don't affect functionality.

Likewise, it would take me a little while to test out the cache context thing at this point, and since we're using this in an admin capacity I don't think it really makes a difference, even though it would be the correct thing to do.

wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new10.9 KB

For some reason, the patch didn't apply, despite no commits having happened. Here's a straight reroll.

wim leers’s picture

StatusFileSize
new11.82 KB
new1.81 KB

Implemented #5.

wim leers’s picture

Status: Needs review » Fixed

Dammit, now I understand why it didn't apply: I didn't know Jesse Beach had pushed another commit! I thought I was on latest, but obviously I wasn't. So, #2 did apply, but the #8 interdiff is still relevant.

Sorry for the long delay, but this is now finally done!

  • Wim Leers committed b083255 on 8.x-1.x authored by TR
    Issue #2346217 by TR, Wim Leers, BarisW: Update code to reflect latest...
tr’s picture

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.