Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

jQuery dialog tidy up code in webtrees1.5.1.js #61

Closed
wants to merge 1 commit into from
Closed

jQuery dialog tidy up code in webtrees1.5.1.js #61

wants to merge 1 commit into from

Conversation

ddrury
Copy link
Contributor

@ddrury ddrury commented Dec 20, 2013

No description provided.

@ddrury
Copy link
Contributor Author

ddrury commented Dec 20, 2013

Rationalize code around the modal dialog functionality. Help dialog loads data from help_text.php by jQuery.getJSON thus cutting calls from two to one.

@ddrury ddrury closed this Dec 20, 2013
@ddrury ddrury reopened this Dec 20, 2013
@ddrury
Copy link
Contributor Author

ddrury commented Dec 20, 2013

Clicked the close button by mistake!! so reopened

Help calls now use JSON to get content - eliminates 2nd ajax call to get
title.
Tidies up modal dialog usage
webtrees js now lint compliant (although more work to be done
@fisharebest
Copy link
Owner

The change to the help dialog (to use a JSON response) is a good idea.

However, you've also made lots of other "clean up" changes at the same time. These make it difficult to review/accept the actual change, as I think these other changes have some errors.

The first cleanup change is this:

  function closePopupAndReloadParent(url) {
   if (parent.opener) {
-    if (url == null || url == "") {
+    if (url === null || url === "") {

This is some old/bad code to detect an optional parameter. If called with no argument, url is undefined. This passes the original test, but not the replacement text. A better fix might be something like:

  if (typeof url === 'undefined') {

Also, at the end of the file, some UTF8 characters are lost. Perhaps your text editor does not handle UTF8 characters? (Meanwhile, I'll replace them with escape sequences...)

Now, patches that simply tidy up code are welcome - but one change per patch makes it much easier to review/manage/accept/etc. Can you submit a separate patch with just the help-dialog changes? Thanks.

@fisharebest fisharebest closed this Jan 4, 2014
@ddrury ddrury deleted the Modal_dialog_tidy branch January 4, 2014 15:09
@ghost ghost mentioned this pull request Aug 9, 2020
@webtrees-pesz webtrees-pesz mentioned this pull request Aug 18, 2023
@makitso makitso mentioned this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants