Hook up global alerts for New Homepage using current sitewide alert system

[djanelle-mit]

Developer

This work connects the sitewide alert system that currently exists to use the new global alert style we introduced as a placeholder template for the new homepage.

Note: the intended behavior here is that for any v2 page (which is currently ONLY the new homepage) the alerts would be styled using the global alert banner pattern. For any other page it will use the current v1 system instead. Both are hooked up to the same post and local storage to remember dismissing should work for both.

To get this to render, this work introduces if statements based on a bool when a page has the v2-page class in the body. These checks are used to change between v1 and v2 versions for the logic to:

• Render the alert template
• Render the close button with the correct markup
• Set local storage and close the alert

Relevant ticket:
https://mitlibraries.atlassian.net/browse/LHNU-238

Stylesheets

• Any theme or plugin whose stylesheets have changed has had its version
  string incremented.

Secrets

• All new secrets have been added to Pantheon tiers
• Relevant secrets have been updated in Github Actions
• All new secrets documented in README

Documentation

• Project documentation has been updated
• No documentation changes are needed

Accessibility

• ANDI or Wave has been run in accordance to
  our guide and
  all issues introduced by these changes have been resolved or opened as new
  issues (link to those issues in the Pull Request details above)

Stakeholder approval

• Stakeholder approval has been confirmed
• Stakeholder approval is not needed

Dependencies

YES | NO dependencies are updated

Code Reviewer

• The commit message is clear and follows our guidelines
  (not just this pull request message)
• The changes have been verified
• The documentation has been updated or is unnecessary
• New dependencies are appropriate or there were no changes

[matt-bernhardt]

I've got comments, but ultimately this seems fine to me. My biggest concern is that we're repeating id values if we ever define more than one alert at the same time - but this is not a condition I can ever recall having, so I'm not sure it makes sense to redevelop this to support hypothetical futures (if you want to, I'll support it, but to be clear I support this merging as-is)

Moving the definition of isV2page into a constant that multiple JS features might rely on is more of a future-us step, so long as we remember to do this when appropriate. The future I'd like to avoid is having other JS features hanging off a global variable defined here - but for now this is fine.

[matt-bernhardt]

Non-blocking comment:
This is probably a good candidate for a constant, rather than a variable (although a variable that is known to be a unique name like this is also probably fine.

The question I'm grappling with is whether a read-only javascript constant like this might be useful in other contexts, for any other javascript-based feature that will need to be operational through the transition between v1 and v2 pages. The first candidates that come to mind are the map and the calendar, but those are each only on a single page - so any issues we run into with those features can be bundled into the migration of those templates.

The various hours widgets will need to run simultaneously on v1 and v2 pages, so depending on whether those widgets change significantly that's a candidate.

For the moment, this is likely fine - but as we get deeper into the transition this line might be something that we should move to functions.php and to spit out a constant as in https://github.com/MITLibraries/mitlib-wp-network/blob/master/web/app/themes/mitlib-parent/functions.php#L238 .

[djanelle-mit]

Ahh interesting... Maybe we couple moving this to a constant with whatever the next feature is that needs to check for this condition. I haven't seen that "create a JS const via PHP" mechanism before!

[matt-bernhardt]

Non-blocking comment:
I'm realizing as I see the alert you created that having a hard-coded value like this isn't particularly robust to different content lengths. I'm not sure what a better option would be offhand, and I'm not asking that we delay this PR while dealing with an existing issue like this, though.

This is also something to consider, as the hours page migrates, whether we need separate setClosable functions that are dedicated to v1 and v2. At the moment with two branches in the same function, the complexity of this function is probably at the limit of what I'd want to support long term. Adding a third branch would push me to recommending we split the call to setClosable on line 143 into something like:

		// If this is a closable alert
		if (true === alert_posts_arr[i].meta.closable) {
            if (isV2page) {
			    setClosableV2(alert_ID);
            } else {
			    setClosableV1(alert_ID);
            }
		}

(This introduces a separate maintenance concern as we'd be maintaining two functions in parallel, but I'd rather that than a single function that branches more than twice - or maybe there's another more efficient mechanism somehow, if we prototype a bit more)

[matt-bernhardt]

Comment:
As you've noted, this is invalid markup if there are ever more than one alert at a time - but this is outside of our anticipated use so I'm not sure that this is something I'm going to ask be changed here.

I do suspect that the use of .closest will end up helping, but ultimately not using repeated id values is the better practice.

[djanelle-mit]

I'm also realizing as I look at the code here again, that we already have a duplicate ID problem since for for both v1 and v2 alerts we're adding a link with #close to trigger the dismiss.

This doesn't happen very often, but when I tested multiple alerts the .closest seemed to make the dismiss buttons work on the correct alert (and didn't fire on all of them) so I think we're OK here.

I'm going to leave this as a future-us thing we fix with whatever longer term solution we come to for Alerts.

[matt-bernhardt]

The setTimeout function won't have any impact on v2 pages (I don't think) because the required markup is only present in the v1 template. I don't think this is an issue of functionality, but I wonder if it might be good to note that this is only relevant for alerts in a v1 context?

(I do see that there's a delay in the alert being rendered on the page, which is either due to the API response being parsed, or to some other timeout - but not this block)

[djanelle-mit]

Good call, updating a comment with this.

EDIT: I merged the branch before actually committing this comment, so going to leave this alone for the time being.