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

FFS-2295: Help modal #417

Merged
merged 55 commits into from
Feb 6, 2025
Merged

FFS-2295: Help modal #417

merged 55 commits into from
Feb 6, 2025

Conversation

GeorgeCodes19
Copy link
Contributor

@GeorgeCodes19 GeorgeCodes19 commented Jan 22, 2025

Ticket

Resolves FFS-2295.

  • Update help topic(s) content
  • Add E2E tests
  • Add /help route to use in iframe
  • Add Help link in top nav that triggers the Help Modal
  • Ensuring that clicking the options link in the help banner triggers the Help Modal
  • Ensuring that the "Go Back" button works within the Help Modal
  • Ensuring that closing the Pinwheel modal triggers the Help Banner to appear

Changes

Context for reviewers

Acceptance testing

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

George Byers added 18 commits January 22, 2025 09:03
…working with view partials"""

This reverts commit 2d09705.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to use a component here since there's now a Help nav item in the main nav as well as a link in the Help banner.

<div class="usa-modal__content">
<div class="usa-modal__main padding-0 margin-0 maxw-none-important">
<div class="usa-modal__body padding-0">
<iframe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I've resorted to an iframe here rather coding a bunch of JS. I figured this would more closely resemble our current approach and that we can add custom Mixpanel/New Relic events using Rails helpers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda a bummer to use an <iframe> but I understand the complexity.

@GeorgeCodes19 GeorgeCodes19 marked this pull request as ready for review January 28, 2025 00:07
Copy link
Contributor

Choose a reason for hiding this comment

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

I woudl investigate whether you could do this within the existing flash error/alert system :)


<ol>
<% steps = t("help.show.#{topic}", default: {}).select { |k, _| k.to_s.start_with?('step') } %>
<% steps.each do |step, _| %>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use this _ thing :)

Copy link
Contributor

@tdooner tdooner 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 to me, pending the one last issue I noticed (the feedback form URL)

// Button display handling based on device orientation and screen size
.usa-button {
&.display-block,
&[class*="margin-bottom"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to override the button display within the iframe, can we just give them a custom class name? I find it odd to have a selector like .usa-button[class*="margin-bottom"], since to me the "margin-bottom" class doesn't really have any connection to the "usa-button" class.

Comment on lines 406 to 408
search_keywords: Search your email for keywords like "payroll," "[Payroll Provider Name]," or "company ID."
title: Check your welcome materials
welcome_materials: Your company ID is often in your onboarding documents, welcome email, or payroll setup info.
Copy link
Contributor

Choose a reason for hiding this comment

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

If designers are ok with the order these items are displaying in now, I guess you can ignore this.

Comment on lines 158 to 160
html {
overflow: hidden;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this do anything? I don't think iframe contents can inherit like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does NOTHING. I was experimenting with some things and accidentally left it in there. The solution here was to set the scrolling property on the iframe itself to false. This approached worked, but it's actually deprecated.

Comment on lines 406 to 408
search_keywords: Search your email for keywords like "payroll," "[Payroll Provider Name]," or "company ID."
title: Check your welcome materials
welcome_materials: Your company ID is often in your onboarding documents, welcome email, or payroll setup info.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sad this feedback didn't make it in!

Copy link
Contributor

Choose a reason for hiding this comment

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

Eager for this redundant controller to be refactored away in follow up work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it's actually going to be addressed in the follow up ticket that I'll start on today.

@GeorgeCodes19 GeorgeCodes19 merged commit 859c96e into main Feb 6, 2025
15 checks passed
tdooner added a commit that referenced this pull request Feb 6, 2025
…ow-use-new-database

* origin/main:
  FFS-2295: Help modal (#417)
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.

3 participants