After #2569569: Make modal display plugin use iframe Iframe and Modal display plugin classes became more alike. See comment: https://github.com/drupal-media/entity_browser/pull/111#discussion_r4387...

CommentFileSizeAuthor
#18 2609308_19.patch766 bytesslashrsm
#12 interdiff-2609308-11-12.txt1.13 KBchr.fritsch
#12 evaluate_if_we_can_move-2609308-12.patch8.89 KBchr.fritsch
#11 evaluate_if_we_can_move-2609308-11.patch8.27 KBchr.fritsch
#5 evaluate_if_we_can_move-2609308-5.patch7.97 KBchr.fritsch

Comments

Primsi created an issue. See original summary.

primsi’s picture

primsi commented
Issue summary: View changes
primsi’s picture

primsi commented
Related issues: +#2569569: Make modal display plugin use iframe
slashrsm’s picture

slashrsm commented
Issue tags: +Novice
chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
StatusFileSize
new evaluate_if_we_can_move-2609308-5.patch7.97 KB

I refactored the modal display that it extends the iframe display, because the modal is in the end basically an iframe.

The attached patched is based on #2828778: Dead code in modal display

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Active » Needs review
Issue tags: -Media Initiative

Status: Needs review » Needs work

The last submitted patch, 5: evaluate_if_we_can_move-2609308-5.patch, failed testing.

The last submitted patch, 5: evaluate_if_we_can_move-2609308-5.patch, failed testing.

The last submitted patch, 5: evaluate_if_we_can_move-2609308-5.patch, failed testing.

The last submitted patch, 5: evaluate_if_we_can_move-2609308-5.patch, failed testing.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new evaluate_if_we_can_move-2609308-11.patch8.27 KB

Rerolled

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
StatusFileSize
new evaluate_if_we_can_move-2609308-12.patch8.89 KB
new interdiff-2609308-11-12.txt1.13 KB

Calling DisplayBase::displayEntityBrowser directly from modal to not call iframe logic.

slashrsm’s picture

slashrsm commented
Status: Needs review » Needs work

Code removals! Yay! Generally looks good. Thank you for working on this!

displayentitybrowser() will run almost identical code twice. Once in Modal and once in iFrame. This also includes REEGISTER_JS_CALLBACKS, which will be dispatched twice. We should avoid that.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs work » Needs review

Have you seen my latest comment #12? It should be fixed there

slashrsm’s picture

slashrsm commented

  • slashrsm committed 42c2bb6 on 8.x-1.x authored by chr.fritsch 
    Issue #2609308 by chr.fritsch, slashrsm: Remove redundant code from...
slashrsm’s picture

slashrsm commented
Status: Needs review » Fixed

Committed. Thanks!

slashrsm’s picture

slashrsm commented
Status: Fixed » Needs review
StatusFileSize
new 2609308_19.patch766 bytes

This removed description for auto_open in Modal config form. This patch adds it back.

slashrsm’s picture

slashrsm commented
Status: Needs review » Fixed

Committed.

  • slashrsm committed 3823dc3 on 8.x-1.x 
    Issue #2609308 by slashrsm: Re-add config UI description for auto_open...

Status: Fixed » Needs work

The last submitted patch, 18: 2609308_19.patch, failed testing.

slashrsm’s picture

slashrsm commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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