Comments

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

Let's do it!

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
Status: Active » Needs review
StatusFileSize
new1.46 KB

Not sure if it's good but here is the patch :)

sun’s picture

Oh, we should actually wait until the parent issue has landed — I should have created this issue as postponed, sorry! :-/

Once that is in, your patch will probably work - we will just need to change the #attached syntax.

sun’s picture

Status: Needs review » Needs work
jibran’s picture

Status: Needs work » Needs review
StatusFileSize
new789 bytes
new1.45 KB

Reroll

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good.
Manually tested and JS does what it is supposed to.
RTBC

Screenshot from manual testing https://www.dropbox.com/s/mit4nwd7dyyou1i/Screenshot%202014-03-11%2016.5...

sun’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/block/custom_block/custom_block.libraries.yml
    @@ -0,0 +1,5 @@
    +  version: 1.0
    

    For all libraries of modules in core, the value of version should be VERSION

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
    @@ -140,9 +140,6 @@ public function form(array $form, array &$form_state) {
    -      '#attached' => array(
    ...
           '#access' => $block->isNewRevision() || $account->hasPermission('administer blocks'),
    

    It looks like the JS was only conditionally loaded previously?

    #access causes the entire element to not be rendered, so #attached libraries are not loaded if #access is FALSE.

  3. We're missing the declaration of dependencies for the library. AFAICS, the dependencies are:

    - core/drupal.form
    - core/drupal
    - core/jquery

jibran’s picture

Status: Needs work » Needs review
StatusFileSize
new1.59 KB
new1.51 KB

Fixed #7 and moved the JS file into JS folder.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2205155-create-custom-block-library-8.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
jibran’s picture

Status: Needs review » Reviewed & tested by the community

Reverting the old status.

wim leers’s picture

I'd have expected the JS (and therefore also the library) to contain the string "admin", because it's not end-user facing, only admin-facing. I could see that being considered out of scope though, so I won't un-RTBC. At the same time, it'd be easy and fine to do that here.

wim leers’s picture

Issue tags: +JavaScript
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f45195e and pushed to 8.x. Thanks!

  • Commit f45195e on 8.x by alexpott:
    Issue #2205155 by jibran, rteijeiro: Convert custom_block.js into a...

Status: Fixed » Closed (fixed)

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