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

New guidance and subpages, and return to previous pages. #1949

Conversation

heyitsmebev
Copy link
Contributor

@heyitsmebev heyitsmebev commented Sep 19, 2024

A note to PR reviewers: it may be helpful to review our code review documentation to know what to keep in mind while reviewing pull requests.

Description

In order to view the new content changes, navigate to /best-practices

This PR is intended to create a new /best-practice directory

Figma: https://www.figma.com/proto/aAWhlPzOP6kfVFnCCAfSZZ/notify.gov?page-id=2339%3A3817&node-id=4964-11162&viewport=2309%2C3419%2C0.25&t=JxwGA3dBSMSAqlqh-1&scaling=min-zoom&content-scaling=fixed&starting-point-node-id=4854%3A212&show-proto-sidebar=1

Related issue: #1928

TODO (optional)

  • Should include a side navigation

  • Should include breadcrumbs

  • Implement guidance behind the log-in (permissions)

  • Should include approved content on each page

  • Should meet our accessibility standards

  • Should provide anchor links on sub pages

  • Layout should be flexible for both mobile and desktop views

@heyitsmebev heyitsmebev linked an issue Sep 19, 2024 that may be closed by this pull request
4 tasks
@heyitsmebev heyitsmebev changed the title Navigate easily to guidance and subpages, and return to previous pages. New guidance and subpages, and return to previous pages. Sep 19, 2024
@heyitsmebev heyitsmebev self-assigned this Sep 19, 2024
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank you, @heyitsmebev and @alexjanousekGSA, wow! 🎉 This is awesome to see!

I'm really happy with the approach here: it's lightweight and non-obtrusive and wouldn't take much to add other feature flags in the future. 🙂 And you have tests! 🎉

There are two things that I did note that I think we should adjust though, and that's how we're reading in the feature flag environment variable initially and how we use it throughout the rest of the code. I have left an explanation with suggestions throughout the review, hopefully it's easy to follow! If it's not, please ask questions and I'll explain further or find time to pair.

There were a few other things I noted as well but not as critical as that piece. Again, very well done and thank you for incorporating a bit of the sandbox management in there as well! 🎉

app/config.py Outdated Show resolved Hide resolved
app/main/views/index.py Outdated Show resolved Hide resolved
app/main/views/index.py Outdated Show resolved Hide resolved
app/main/views/index.py Outdated Show resolved Hide resolved
deploy-config/demo.yml Outdated Show resolved Hide resolved
tests/app/main/views/test_index.py Outdated Show resolved Hide resolved
tests/app/main/views/test_index.py Outdated Show resolved Hide resolved
app/assets/javascripts/subNav.js Outdated Show resolved Hide resolved
app/assets/javascripts/subNav.js Outdated Show resolved Hide resolved
app/assets/javascripts/subNav.js Outdated Show resolved Hide resolved
Copy link
Contributor

@terrazoon terrazoon 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. I think that if we are adding a new feature with new nav, there should be a new e2e test to cover that. This would be a great place to start doing that.

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank you, @heyitsmebev and @alexjanousekGSA, for making all of the adjustments! 🎉

@alexjanousekGSA I also saw your note about when the environment variable is first read in to the app config as well. It may still be possible to simplify this but you're also right that any non-empty string value in Python evaluates to True when doing a boolean conversion. I see now that the statement that assigns the value to the app config variable is the result of evaluating the equality check of the strings, so I'm fine with it as is for now. 🙂

@alexjanousekGSA alexjanousekGSA merged commit 60e9ac1 into main Oct 31, 2024
11 checks passed
@alexjanousekGSA alexjanousekGSA deleted the 1928-develop-new-page-templates-in-code-to-sit-behind-the-log-in branch October 31, 2024 16:49
@heyitsmebev
Copy link
Contributor Author

e2e for this feature is here #2074

@heyitsmebev heyitsmebev linked an issue Oct 31, 2024 that may be closed by this pull request
4 tasks
@heyitsmebev heyitsmebev linked an issue Oct 31, 2024 that may be closed by this pull request
11 tasks
@heyitsmebev heyitsmebev linked an issue Nov 7, 2024 that may be closed by this pull request
2 tasks
@heyitsmebev heyitsmebev linked an issue Nov 7, 2024 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants