Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

close button is missing from modal dialog #217

Open
soniktrooth opened this issue Nov 3, 2015 · 10 comments
Open

close button is missing from modal dialog #217

soniktrooth opened this issue Nov 3, 2015 · 10 comments
Labels
Milestone

Comments

@soniktrooth
Copy link
Contributor

This missing close dialog led me to discover a name space collision for .button() method. Both Bootstrap and jQueryUI implement this method.

screen shot 2015-11-03 at 10 19 40 am

@soniktrooth soniktrooth changed the title Close icon disappears No close icon on dialog Nov 3, 2015
@soniktrooth
Copy link
Contributor Author

It comes down to the fact that the markup is different:

<div class="ui-dialog-titlebar ui-widget-header ui-corner-all ui-helper-clearfix">
  <span id="ui-id-1" class="ui-dialog-title">&nbsp;</span>
  <button class="ui-dialog-titlebar-close"></button>
</div>

vs

<div class="ui-dialog-titlebar ui-widget-header ui-corner-all ui-helper-clearfix">
  <span id="ui-id-2" class="ui-dialog-title">&nbsp;</span>
  <button class="ui-button ui-widget ui-state-default ui-corner-all ui-button-icon-only ui-dialog-titlebar-close" role="button" aria-disabled="false" title="close">
    <span class="ui-button-icon-primary ui-icon ui-icon-closethick"></span>
    <span class="ui-button-text">close</span>
  </button>
</div>

And I think it comes down to this piece of code from jquery.ui.dialog.js:

        this.uiDialogTitlebarClose = $("<button></button>")
            .button({
                label: this.options.closeText,
                icons: {
                    primary: "ui-icon-closethick"
                },
                text: false
            })
            .addClass("ui-dialog-titlebar-close")
            .appendTo( this.uiDialogTitlebar );

@soniktrooth
Copy link
Contributor Author

I think this is a name space clash. Both Bootstrap and jQuery UI have a .button() method. Removing BS from the page lets the code in jquery.ui.dialog.js run properly and gives us the correct markup.

@andrewmallis
Copy link
Member

@soniktrooth
Copy link
Contributor Author

@andrewmallis I don't believe that's the same issue. Some little css tweaks fixed their problem but ours is that the markup is completely different—and it's the .button() method that is adding the required markup.

@soniktrooth soniktrooth changed the title No close icon on dialog Namespace collision on .button() Nov 3, 2015
@soniktrooth
Copy link
Contributor Author

Here are some options:

We could use the widget bridge included in jQuery UI as outlined here: http://www.ryadel.com/en/using-jquery-ui-bootstrap-togheter-web-page/ however we have no control over how it's called in Panopoly so I don't think this will work.

So I'm leaning towards this:

var btn = $.fn.button.noConflict() // reverts $.fn.button to jqueryui btn
$.fn.btn = btn // assigns bootstrap button functionality to $.fn.btn

This will mean that any time we want to use the button method from BS we use .btn() instead. Not great because it doesn't match up with the BS docs but I don't think there's a better way around this. We will have to document it well.

We will have to do the same thing for .tooltip() because it is implemented by both libraries too.

@andrewmallis andrewmallis changed the title Namespace collision on .button() close button is missing from modal dialog Nov 3, 2015
@andrewmallis
Copy link
Member

@soniktrooth I'd like if we can keep the issue descriptions more focused on the problem that is exhibited, as this will help users with the same problem identify the the issue.

@andrewmallis
Copy link
Member

I am not excited to break the documented bootstrap pattern in principal. I lack js fluency to propose anything concrete, but perhaps @RobLoach may be of assistance here.

I do recall in the vestigial 4.x branch we managed to replace the modals with BS modals straight up. Please see SHA 5644548

Also, please see this issue: https://www.drupal.org/node/2546874
which I believe is related, and does bring a solution to radix, another BS based theme that plays nice with Panopoly.

@soniktrooth
Copy link
Contributor Author

I'm not excited about changing the documented BS patterns either but there's not a lot of options for two libraries to co-exist. Ultimately, replacing jQueryUI modals with BS modals would be ideal, and we'd have to look at tooltips too and investigate if there are any other namespace collisions to address.

I will look into the radix solution you linked to—if we can fix this with js file ordering for now that would be great as I was hoping to do a 'bring in the goodness from 4.x' sprint at some stage soon.

@soniktrooth
Copy link
Contributor Author

The load ordering is sorted by #219

@soniktrooth soniktrooth assigned RobLoach and unassigned soniktrooth Mar 16, 2016
@RobLoach RobLoach removed their assignment Oct 9, 2017
@ghost
Copy link

ghost commented Feb 20, 2018

i have the same issue on d8, please help me.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants