Skip to content

LG-3711: Migrate ad hoc alerts to USWDS alert partial#4418

Merged
aduth merged 12 commits intomasterfrom
aduth-ad-hoc-alerts
Dec 2, 2020
Merged

LG-3711: Migrate ad hoc alerts to USWDS alert partial#4418
aduth merged 12 commits intomasterfrom
aduth-ad-hoc-alerts

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 18, 2020

Related: LG-3711
Previously: #4375

Why: Consolidate to single, consistent alert styling.

I've had this branch sitting locally for some time, initially as an exploration of effort involved in migrating the remainder of ad-hoc alert usage.

A few notable findings:

@aduth
Copy link
Contributor Author

aduth commented Nov 27, 2020

  • Following the logic, there may be an issue prior to these changes where SP custom alert texts may not be showing correctly in non-English locales. Needs more investigation.

In further testing, this didn't prove to be a problem after all. The initial worry was the difference in how we consider paths between...

sign_in_path =
I18n.locale == :en ? new_user_session_path : new_user_session_path(locale: I18n.locale)

<% if current_page?(new_user_session_path) %>

But it appears that by the time the view is rendered, the new_user_session_path takes current locale into consideration, which is not the case in the decorator.

@aduth aduth force-pushed the aduth-ad-hoc-alerts branch from 2604788 to 48ba7c5 Compare November 27, 2020 16:30
@aduth
Copy link
Contributor Author

aduth commented Nov 27, 2020

Updates:

  • The SimpleForm customization we need seems to be only achievable by monkey-patch (6518e3c, a783a7aef). I landed at this after a thorough exploration of the relevant source revealed no additional options for markup customization, and in referencing relevant third-party CSS framework integration documentation suggesting a similar approach. A primary alternative here is to avoid using error_notification altogether, if I'm correct in assuming there's no magic error notification display logic and that we're only using it in a single form (source).
  • Remove _alert.scss (48ba7c554) 🎉. I'd have liked to remove all of the alert icon images, but it appears we use some of them elsewhere. I think those should be updated at some point to use the design system icons, so we have consistency.

@aduth aduth marked this pull request as ready for review November 27, 2020 16:56
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Contributor Author

aduth commented Nov 27, 2020

Fortunately the failing tests related to outdated selectors in ec77b7f2a clued me into a regression I'd accidentally introduced, where previously we relied on the decorator's sp_msg method to interpolate some common variables into the message. Since I adapted custom_alert to return the actual alert message, this wasn't happening anymore. Rather than worrying about keeping them in sync, I removed sp_msg and generate_custom_alert, and just made custom_alert return the formatted text.

Updated in 313f1c4e0.

It also prompted me to figure out why my changes to config/service_providers.yml weren't taking effect (needed make setup) so that I could confirm it actually works locally:

Screen Shot 2020-11-27 at 1 49 10 PM

@aduth
Copy link
Contributor Author

aduth commented Nov 27, 2020

I was having some trouble with tests in the previous implementation having the monkey-patch at lib/simple-form. It appeared that the very presence and require of the file was preventing the base class from being defined at all, where the intention was to extend it. Loosely following the convention of Active Support Core Extensions (lib/core_extensions), it appears to be happier at lib/extensions/simple-form.

I might very well be missing something obvious about load order / auto-loading / monkey-patching. Happy to adjust if I am, or if there might be a better place to put these files than creating a new directory.

aduth added 12 commits November 27, 2020 15:50
**Why**: Consolidate to single, consistent alert styling
**Why**: Only option to customize the rendered markup of alert to support nested alert tags
**Why**: So next developer understands what's up
**Why**: From `generate_custom_alert`, easier to understand
**Why**: Broke with refactoring to sp_alert to avoid sp_msg. Since sp_msg was otherwise unused, absorb behavior into custom_alert as single source of (interpolated) message text.
**Why**: My own self-assurance that I'm not breaking things
**Why**: Allow base class to be defined for extension in tests
@aduth aduth force-pushed the aduth-ad-hoc-alerts branch from 3cb35f4 to 55f0a4d Compare November 27, 2020 20:50
@aduth aduth changed the title Migrate ad hoc alerts to USWDS alert partial LG-3711: Migrate ad hoc alerts to USWDS alert partial Dec 2, 2020
@aduth aduth merged commit 99959f5 into master Dec 2, 2020
@aduth aduth deleted the aduth-ad-hoc-alerts branch December 2, 2020 20:37
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.

2 participants