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.

Sign up for GitHub

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

Jump to bottom

TUP-703 TACC Home Banner Links Open in New Window #805

Merged
merged 7 commits into from
Mar 1, 2024
Merged

TUP-703 TACC Home Banner Links Open in New Window #805

merged 7 commits into from
Mar 1, 2024

Conversation

R-Tomas-Gonzalez
Copy link
Collaborator

Overview

Prevent banner links from opening in new windows.

Related

Changes

Changes what is considered an external link. Looks for the host name with or without www., then opens in new window if it isn't matching to the document's host.

Testing

A) If you'd like to troubleshoot in TUP-UI Repo:

  1. Comment out code on Core CMS
  2. Follow https://github.com/TACC/Core-CMS/wiki/Locally-Develop-CMS-and-TUP-CMS
  3. Make snippet with code in TUP -> look at attachment for example
  4. Add snippet to Django and import it in the footer

B) If you'd like to just test these changes in TUP-UI using Core_CMS

  1. Follow https://github.com/TACC/Core-CMS/wiki/Locally-Develop-CMS-and-TUP-CMS

UI

…

Notes

The shouldForceSetTarget function below:

      const shouldForceSetTarget = pathsToForceSetTarget.some(path => {
        let shouldForce;
        if (path instanceof RegExp) {
          shouldForce = path.test(link.pathname);
        }
        if (typeof path === 'string') {
          shouldForce = _doPathsMatch(path, link.pathname);
        }
        if (SHOULD_DEBUG && shouldForce) {
          console.debug(`Path "${link.pathname}" matches "${path}"`);
        }
        return shouldForce;
      });

does not allow external links to open in a new window.

For instance. If you're in localhost, and have a tacc.utexas.edu link - it should open in a new window because it doesn't match the host.

This might be something you test in tup-ui repo using the A) testing steps above

wesleyboar
wesleyboar reviewed Feb 29, 2024
View reviewed changes
taccsite_cms/static/site_cms/js/modules/setTargetForExternalLinks.js Outdated
Comment on lines 61 to 62
//shouldForceSetTarget could be used in this if statement if passing in options.
if (!isInternalLink || isMailto ) {
Copy link
Member

Choose a reason for hiding this comment

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

Yup. (Thanks for the comment/reminder.) We avoid shouldForceSetTarget for now, cuz it causes the bug.

I'll check that no client needs that feature, and if one does, how to not require this script to handle that.

Copy link
Member

@wesleyboar wesleyboar Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update: No client needs shouldForceSetTarget. Celebrate with Fire: You may burn all code and variables relevant to it e.g. pathsToExernalSite, pathsToForceSetTarget. Bonus: You may clean up even more cruft code, i.e. perform CMD-87, at your leisure, now or later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. 72e3d15

wesleyboar
wesleyboar reviewed Feb 29, 2024
View reviewed changes
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Looks good. Please delete shouldForceSetTarget and related code.

taccsite_cms/static/site_cms/js/modules/setTargetForExternalLinks.js Outdated
Comment on lines 61 to 62
//shouldForceSetTarget could be used in this if statement if passing in options.
if (!isInternalLink || isMailto ) {
Copy link
Member

@wesleyboar wesleyboar Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update: No client needs shouldForceSetTarget. Celebrate with Fire: You may burn all code and variables relevant to it e.g. pathsToExernalSite, pathsToForceSetTarget. Bonus: You may clean up even more cruft code, i.e. perform CMD-87, at your leisure, now or later.

wesleyboar
wesleyboar reviewed Feb 29, 2024
View reviewed changes
taccsite_cms/static/site_cms/js/modules/setTargetForExternalLinks.js Outdated
Comment on lines 1 to 5
/**
* Whether to log debug info to console
* @const {string}
*/
const SHOULD_DEBUG = window.DEBUG;
Copy link
Member

Choose a reason for hiding this comment

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

Woah. Control the burn. The SHOULD_DEBUG is still used. Also, the jsDoc block (a docblock) is still accurate.

Copy link
Collaborator Author

@R-Tomas-Gonzalez R-Tomas-Gonzalez Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry. Click this: 🔥

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Restored via cbad538

taccsite_cms/static/site_cms/js/modules/setTargetForExternalLinks.js Outdated
Comment on lines 13 to 20
/**
* Set external links (automatically discovered) to open in new tab
* @param {object} [options] - Optional parameters
* @param {array.<string>} [options.pathsToExernalSite=[]] - (DEPRECATED) A list of relative URL paths that should be treated like external URLs
* @param {array.<string|RegExp>} [options.pathsToForceSetTarget=[]] - A list of relative URL paths (or patterns) that should trigger setting a target
* @param {HTMLElement|Document} [options.scopeElement=document] - The element within which to search for links
* @param {setTargetCallback} [options.setTargetCallback] - A callback for after a target is set
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please retain the docblock sans @param lines. The function description is still accurate.

Docblocks are written in jsDoc format. They provide manual definitions (in lieu of TypeScript), and can be used to auto-generate documentation e.g. vite.js on jsdocs.io.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Restored via cbad538 without params.

@wesleyboar
chore: remove unused jsDoc bit
9b2a0a5 
I msitakenly requested this be restored. My bad.

(First time using Github on phone, so I misread.)

https://github.com/TACC/Core-CMS/pull/805/files#r1509226199
@wesleyboar wesleyboar merged commit 11e66dd into main Mar 1, 2024
@wesleyboar wesleyboar deleted the bug/tup-703-home-banner-links-new-window branch March 1, 2024 16:28
This was referenced Mar 2, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleyboar wesleyboar wesleyboar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@R-Tomas-Gonzalez @wesleyboar