Open the "discard changes" dialog in the off-canvas tray

Created on 9 July 2021, over 3 years ago
Updated 18 September 2024, 3 months ago

Problem/Motivation

If you currently click on the "Discard changes" button in the Layout Builder, you will be forwarded to a separate page to confirm the dialog. I think it's the same for the "Revert to Default" button.
I'm wondering if there's a particular reason why these dialogs are opened on a separate page.

Otherwise, wouldn't it be a better UX to open those dialogs in the off-canvas tray like all the other Layout Builder dialogs to keep things consistent?

Steps to reproduce

  1. Create a layout with the Layout Builder.
  2. Click on the "Discard changes" button in the Layout Builder.
  3. Find yourself on a page with URL pattern /node/[nid]/layout/discard-changes to confirm the dialog.

Proposed resolution

I'd expect those dialogs to be opened in the off-canvas tray.

Remaining tasks

Discuss if opening those dialogs in the off-canvas tray is a good solution.

User interface changes

TBD.

API changes

TBD.

Data model changes

TBD.

Release notes snippet

TBD.

✨ Feature request
Status

RTBC

Version

11.0 πŸ”₯

Component
Layout builderΒ  β†’

Last updated 1 day ago

Created by

πŸ‡©πŸ‡ͺGermany design.er

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡§πŸ‡ͺBelgium StryKaizer Belgium

    Attached is a fix which opens both discard changes and revert to defaults in a dialog.

    I added an extra patch for people who use gin/gin layout builder, which ensures the buttons are rendered nicely.

  • Status changed to Needs review 9 months ago
  • πŸ‡§πŸ‡ͺBelgium tim-diels Belgium πŸ‡§πŸ‡ͺ

    The patch works as expected.

    But can we do the width of the modal different? I looked up in Drupal core and mostly 880 px is used.
    The FieldUiLocalAction is using 85vw which seems much better to implement then hard pixels?

    The normal patch is probably what is needed and a follow up ticket will be needed in Gin Layout Builder to cover the classes on the button?

  • Status changed to Needs work 9 months ago
  • πŸ‡§πŸ‡ͺBelgium tim-diels Belgium πŸ‡§πŸ‡ͺ

    Still small nitpick:

    +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
    @@ -193,21 +194,27 @@ protected function actions(array $form, FormStateInterface $form_state) {
    +    $actions ['discard_changes'] = [
    ...
    +    $actions ['revert'] = [
    

    There must be no space before the bracket.

  • First commit to issue fork.
  • πŸ‡¨πŸ‡¦Canada kalanh

    The patch from #7 no longer applies, but I think this would be a sensible UX enhancement. I have pushed an issue fork and attached a patch for gin_lb users.

  • Pipeline finished with Failed
    4 months ago
    Total: 189s
    #251940
  • Pipeline finished with Failed
    4 months ago
    Total: 187s
    #252013
  • Pipeline finished with Failed
    4 months ago
    Total: 430s
    #252165
  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 149s
    #255271
  • Pipeline finished with Canceled
    4 months ago
    Total: 1925s
    #255277
  • Pipeline finished with Failed
    4 months ago
    Total: 271s
    #258632
  • Pipeline finished with Failed
    4 months ago
    Total: 132s
    #258713
  • Pipeline finished with Failed
    4 months ago
    Total: 3726s
    #258725
  • Pipeline finished with Failed
    4 months ago
    Total: 263s
    #261855
  • Pipeline finished with Success
    4 months ago
    Total: 431s
    #261880
  • πŸ‡¨πŸ‡¦Canada man-1982

    Looks like MR ready to review!
    Dear Colleagues please you take a look.
    Thanks

  • Status changed to Needs review 4 months ago
  • πŸ‡¨πŸ‡¦Canada man-1982

    I've prepared some proof that MR does work. I used clean installation Drupal 11.

    thanks

  • Hi,
    I've reviewed the MR !9183 on Drupal 11.x.

    Layout Discard changes opened in the dialog box.

  • Status changed to Needs work 4 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Just checking issue summary it appears to be incomplete, UI changes should be placed

    Left some comments on the MR

  • Pipeline finished with Canceled
    4 months ago
    #272901
  • Pipeline finished with Canceled
    4 months ago
    Total: 91s
    #272902
  • Pipeline finished with Canceled
    4 months ago
    Total: 91s
    #272903
  • Pipeline finished with Failed
    4 months ago
    Total: 685s
    #272904
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    If they’re new functions being added type hints should be included. Unless it’s a base method that would require updating all spots.

  • πŸ‡¨πŸ‡¦Canada man-1982

    Hi @smustgrave!
    Could you provide a couple examples or give me direction, where i can have a look and read about

    type hints should be included

    I didn't clear understand your message.

    Thank you.

  • Pipeline finished with Canceled
    3 months ago
    Total: 286s
    #285521
  • Pipeline finished with Success
    3 months ago
    Total: 574s
    #285525
  • Status changed to Needs review 3 months ago
  • Reviewed latest MR ,it applies cleanly and MR resolves mentioned issue.

    Drupal version 11.x-dev

    1. Create a layout with the Layout Builder.
    2. Click on the "Discard changes" button in the Layout Builder.
    3. Before MR, it would redirect to /node/[nid]/layout/discard-changes to confirm the dialog
    4. After applying MR is opens in dialogue box.
    5. Attaching screenshot for before and after.
    Issue can be moved forward.
    RTBC +

  • Status changed to RTBC 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Feedback from #20 appears addressed

  • πŸ‡¨πŸ‡¦Canada kalanh

    I've attached the gin-lb version patch for d10.3:

  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    smustgrave β†’ changed the visibility of the branch 11.x to hidden.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    smustgrave β†’ changed the visibility of the branch drupal-3223022-3223022-modal-discard-dialog to hidden.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Rebased but hiding the patch as that triggered the bot.

  • Pipeline finished with Canceled
    2 months ago
    Total: 415s
    #305529
  • Pipeline finished with Failed
    2 months ago
    Total: 175s
    #305540
  • Pipeline finished with Failed
    2 months ago
    Total: 132s
    #305546
  • Pipeline finished with Failed
    2 months ago
    Total: 177s
    #305558
  • Pipeline finished with Success
    2 months ago
    Total: 648s
    #305561
  • πŸ‡¨πŸ‡¦Canada man-1982

    Hi gays,

    i'de like to ask what we decided about patch?
    Do we need more extra tests or works?

    it would be my pleasure to help with this one.

    thanks

Production build 0.71.5 2024