search

department-of-veterans-affairs / va.gov-team

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
275 stars 193 forks source link

PW intake: Correct external link behavior based on updates to design standards #81506

Open mnorthuis opened 1 month ago

mnorthuis commented 1 month ago

Describe your request

A decision was made to change the behavior for handling external links within the design system. The change to the standards direction is documented in issue #1680 within the design system repo.

Previously the content styleguide that required certain sub-domains to open in a new tab ((listed here):

Content example links point to CMS content search with Published nodes that link to these subdomains:

There is code in content-build that says: for a specific set of subdomains, force any links to those subdomains to open in a new tab: https://github.com/department-of-veterans-affairs/content-build/blob/main/src/site/stages/build/plugins/modify-dom/update-external-links.js

We need to remove the rule(s) / code that enable the new tab behavior, so links to these subdomains open in the same tab.

Time constraints / launch deadlines

no hard timing

Your OCTODE product owner

@davidconlon @mnorthuis

Acceptance Criteria

jilladams commented 1 month ago

Updated ticket body:

There is code in content-build that says: for a specific set of subdomains, force any links to those subdomains to open in a new tab: https://github.com/department-of-veterans-affairs/content-build/blob/main/src/site/stages/build/plugins/modify-dom/update-external-links.js

FranECross commented 3 weeks ago

how to get to content audit: log into drupal (tabs or dropdown); click hamburger; click content audit tools; content search in next tab/dropdown.

jilladams commented 3 weeks ago

While chasing a Facilities new tab issue, I randomly just saw that this subdomain/new tab logic was originally created within a commit labeled "reduce memory consumption on content-build": https://github.com/department-of-veterans-affairs/content-build/commit/29bfb339a0a40e9ed4190502cace4f02c5456411. (From Nick Sullivan in 2021)

Ticket ID was 15601, but that doesn't map to a relevant thing in either va.gov-cms or va.gov-team, so might go back to the vets-gov days? Meaning: I can't find the history of why this kind of code change mattered for memory.

That makes me nervous, though it's hard to image how opening things in same tab vs. new tab could affect memory consumption? Maybe just rolled into a different commit message / squashing or something. I feel inclined to add an AC to think about memory when we make this change, but I don't know how we would measure memory consumption. Flagging for @dsasser / @randimays awareness, and re-adding Needs refining label just in case. (FYI @FranECross )

jilladams commented 3 weeks ago

Also noting mostly for my own sake: I've been looking at that content-build file as only applying to subdomains. But! further down in file, there's more logic that says: if data-same-tab isn't set, open in new tab.

So: fixing this file to stop new tabs will very likely solve far more than just the subdomain list in ticket body: https://github.com/department-of-veterans-affairs/content-build/blob/main/src/site/stages/build/plugins/modify-dom/update-external-links.js#L49

FranECross commented 3 weeks ago

@jilladams Thanks so much for this history and suggestion of additional AC. I've added!

jilladams commented 2 weeks ago

@FranECross we missed this in Sprint 4 planning. I'm going ahead and tagging into Sprint 5 so we don't lose it for next spring planning. Do you have a Draft sprint plan started for Sprint 5 that we could put it in for visibility?

FranECross commented 1 week ago

@jilladams Thanks for catching this one! I just created a Sprint 5 Plan draft and added this to it. https://github.com/department-of-veterans-affairs/va.gov-cms/issues/18164

randimays commented 4 days ago

Pulling in as stretch (at the verrry end of Sprint 4)

randimays commented 4 days ago

I did some spelunking on the history of this code; my findings are below.

PR that "created" the update-external-links file

https://github.com/department-of-veterans-affairs/vets-website/pull/15601

This PR appeared to create the update-external-links.js file. But it didn't; it moved the file (Github visually treats moving a file like deleting it and creating a new file). The contents of the file were unchanged. 15601 is the number of the PR and doesn't correspond to a ticket.

Note: this work took place before vets-website and content-build were separate repositories. Notice the PR linked above is in vets-website but refers to code that is in content-build today. Spelunking got a little tricky.

The tickets related to this performance work are here:

Quick summary of the tickets

As the Drupal content grew, the PW team at the time proposed and implemented solutions for optimizing HTML-parsing per-template for some performance gains on the content build.

Here's what the content build used to do for HTML-parsing (before these performance enhancements):

Here's what the PW team did to enhance performance:

So what you're trying to say is...

The external links file has a nominal impact on performance. We should be all clear to change or remove it for this ticket and not worry about that.

When was this file created?

https://github.com/department-of-veterans-affairs/vets-website/pull/8906

Here's the PR where this file was created back in November 2018. Unfortunately, it links to a ticket in the vets.gov-team repository that now 404s.

Description in the PR:

This is slightly different from the attached issue, but fixing the links in the build is easier than spitting out a ton of errors and forcing people to fix them manually. Plus, this makes it easier on people creating links in Markdown.

It seems like this PR was intended to more widely address specific editor-controlled issues with incoming links from external domains. I'm guessing the incoming links were inconsistent in their attributes (target, rel, etc.) and it caused build errors.

Phew, ok.

Given all of the above, I'm going to take a close look at the code and make the adjustments we need without worrying about performance impact.

FranECross commented 4 days ago

🕵🏽‍♀️ You are an amazing sleuth, @randimays !!

randimays commented 3 days ago

@mnorthuis @FranECross @jilladams This might be covered in conversations somewhere but I didn't see it here. We don't want to open in the same window if the user could lose progress or data. What if one of these links for a subdomain is on a page where a user would lose progress?

I'm not sure if there are any links coming from the CMS that would cause a user to lose progress or data. I guess my broader question is: how are we planning to handle those, if they exist?

randimays commented 3 days ago

@FranECross @jilladams Is this ticket intended to address all links in Public Websites products on Drupal and React pages? Or do we have tickets for that work already?

FranECross commented 3 days ago

@randimays My brain is foggy on what we discussed when the Spike was finished. I 'think' it was that if there were enough links in various products that warranted more than one ticket, then I would (using direction from engineers) create those tickets and we could chunk up the work in a way that made sense.

@jilladams Does your brain remember this or can you shed any other info I've missed or forgotten? I thought I'd put notes in the Spike but don't see any.

mnorthuis commented 3 days ago

@randimays in that scenario it really doesn't matter if its an internal link, subdomain link, or external link. Guidance is for teams to avoid placing links somewhere that creates a way for a user to leave a critical flow. If that cannot be avoided for some reason, then they can override and force a new tab with proper content/labeling.

This should not be an issue inside CMS given it does not house any flows or dynamic pages, which is where this scenario would potentially happen.

jilladams commented 2 days ago

@mnorthuis question: Just reread https://design.va.gov/content-style-guide/links#linking-to-external-sites and the guidance doesn't actually mention anything about either the new or the old guidance anymore:

Is that wrong? Can you verify the "new" guidance is still what we're aiming for here, and/or clarify why it's not publicly listed any longer in the style guide?

mnorthuis commented 2 days ago

The content team removed the behavior guidance from the content style guide and only refers to the content direction. The link behavior is found with the link component and references the new guidance.

It might be helpful for them to link to the link component for implementation and behavior guidance. I'll talk with them about that.

randimays commented 2 days ago

We had a huddle this morning to be sure we understand the technical impact of making changes to this file (as it applies to every page on VA.gov). Confirmed understanding with @jtmst and posted a summary in this Slack thread: https://dsva.slack.com/archives/C52CL1PKQ/p1717089115627859

randimays commented 2 days ago

Draft PR posted: https://github.com/department-of-veterans-affairs/content-build/pull/2110 I have tested the affected links with prod content locally and they are behaving as expected (opening in the same tab).

We are giving VFS teams some time to ensure our changes will not adversely impact their applications; planning to merge this (barring no other issues) during the week of 6/3.

FranECross commented 2 days ago

Slack message posted to VFS-all-teams asking folks to review the PR and determine if there will be any ill-effects to their features/products.