Skip to content
This repository was archived by the owner on Oct 8, 2021. It is now read-only.

Fix Issue #2084 option to not-include dialog close icon#3260

Closed
dannyc wants to merge 2 commits intojquery-archive:masterfrom
dannyc:master
Closed

Fix Issue #2084 option to not-include dialog close icon#3260
dannyc wants to merge 2 commits intojquery-archive:masterfrom
dannyc:master

Conversation

@dannyc
Copy link
Contributor

@dannyc dannyc commented Dec 13, 2011

Two issues here. First is issue #2084 Dialog close icon is always added even if not desired. Two, some how this slipped through over time- there is an option "close btn txt" but the text is never shown since the iconpos is hardcoded to "notext"!

Solution:
Add two new options:

  1. iconpos for close button- defaults to notext.
  2. includeCloseBtn- defaults to true.

Reasons why a person would not want the close icon button can vary, but window.hisory.back() is now always wanted. Now users can do whatever they want there, no button, button on the right, button with different icon, and attach whatever event handling they want to it.

unknown added 2 commits December 13, 2011 12:45
1) iconpos for close button- defaults to notext.
2) includeCloseBtn- defaults to true.

At moment there is a closeBtnText option, but text will never be shown since iconpos is always notext!
Currently anyone who doesn't want the default close icon button is stuck or must do some catching and hiding of the element. Not so intuitive.  Default close button is not always wanted.  Also, the window.hisory.back() is always wanted.  Now users can do whatever they want.
@johnbender
Copy link
Contributor

@dannyc

Can submit two pull requests? One for the feature that addresses #2084 and one to correct the data-iconpos value. That'll make it easier to land the fix on our stable branch and the feature for the next point release.

Also, if you're feeling adventurous we love when people add tests. In any case thanks for the pull request, and please report back here if you create another two so that we can close this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd probably want to avoid the creation of the element all together on line 18. Otherwise the css suggestion of setting display: none is equivalent.

#2084 (comment)

@johnbender
Copy link
Contributor

@dannyc

I commented on the close btn option alteration, and I'm going to close this in favor of opening two new pull requests for each change. Thanks for the pr and please do resubmit.

@johnbender johnbender closed this Apr 4, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants