-
Notifications
You must be signed in to change notification settings - Fork 70
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
VAGOV-TEAM-97920: Form Builder Navbar #20149
base: main
Are you sure you want to change the base?
Conversation
GitHub Workflows (.github/workflows/*.yml)Have you...
|
Checking composer.lock changes... |
2d942c6
to
d9ec28a
Compare
Checking composer.lock changes... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We already talked in-depth about these changes yesterday, and this looks in-line with what we discussed then. I appreciate the thorough testing.
|
||
<li class="form-builder-navbar__tab form-builder-navbar__tab--forms | ||
{{ | ||
active_tab == 'forms' ? 'form-builder-navbar__tab--active' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish this was less verbose, but it makes sense for guaranteeing that our active styles are isolated to this module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree entirely. I started to abstract it a bit, and it started to feel like something with not enough ROI, at least in the moment. There's not a real good, easy way to do it. But I don't love this at all.
// Successful submission should take user to next page. | ||
$nextPageUrl = $this->getSession()->getCurrentUrl(); | ||
$this->assertStringContainsString('/name-and-dob', $nextPageUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fix for this test.
Checking composer.lock changes... |
7513753
to
ceb6802
Compare
Checking composer.lock changes... |
Checking composer.lock changes... |
- Renames form-builder theme and sets it as page-level theme.
ab92cd4
to
987c603
Compare
Checking composer.lock changes... |
Tugboat has finished building the preview for this pull request! Links:
Dashboard: |
Cypress Accessibility Violations
|
Description
This PR adds a navbar (three tabs) to the Form Builder page template. More importantly, it sets things up to be able to pass custom data from each form to the wrapping page template. In this case, we needed that to be able to pass the
activeTab
value to the page template. It might be used for additional values as we go.Closes department-of-veterans-affairs/va.gov-team#97920
Testing done
Screenshots
QA steps
/form-builder/intro
node/add/digital_form
Definition of Done
Select Team for PR review
CMS Team
Public websites
Facilities
User support
Accelerated Publishing
Form Builder
Is this PR blocked by another PR?
DO NOT MERGE
Does this PR need review from a Product Owner
Needs PO review
CMS user-facing announcement
Is an announcement needed to let editors know of this change?