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

Show "Login.gov" app name in IdV app password confirmation step #6342

Merged
merged 8 commits into from May 13, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented May 12, 2022

Why: So that the user sees "Login.gov" in the browser tab, not "%{app_name}".

Screenshot:

Before After
image image

aduth added 3 commits May 12, 2022 11:02
Let "config" stand for common application values
**Why**: So that the user sees "Login.gov" in the browser tab, not "%{app_name}".

changelog: Upcoming Features, Identity Verification, Add password confirmation step
@aduth aduth requested a review from a team May 12, 2022 15:46
Comment on lines 29 to 31
if (config === undefined) {
config = JSON.parse(document.querySelector('[data-config]')?.textContent || '');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

yay big fan of caching it like this -- would it also make sense to add a method to clear/reset the config value for tests and such?

Copy link
Member Author

Choose a reason for hiding this comment

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

yay big fan of caching it like this -- would it also make sense to add a method to clear/reset the config value for tests and such?

Yeah, turns out we'll need some way to reset it in tests. I went through a few different ideas for how to implement this, and landed at the changes in 7378e6a to exempt caching in the test environment. I had dabbled with a few ideas including resetting via method or property, but the downside is that either each spec file which uses a config value would need to be aware to do the reset, or apply it globally in test setup, neither of which were very appealing to me.

The nice thing about NODE_ENV conditions is that through Webpack source replacement + Terser dead code elimination, the logic gets excluded from production build.

let config;
const isCacheEnvironment = 'production' !== 'test';
function getValue(key) {
  if (config === undefined || !isCacheEnvironment) {
    config = {};
  }
  return config[key];
}

console.log(getValue('example'));
let e;console.log((void 0===e&&(e={}),e["example"]));

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a good compromise! 👍

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 added 5 commits May 12, 2022 13:43
So that we don't need to deal with assuring the element exists in test environments, and to improve tolerance to an invalid page setup
@aduth aduth merged commit f3ed9d5 into main May 13, 2022
@aduth aduth deleted the aduth-app-name-config branch May 13, 2022 14:14
@aduth aduth mentioned this pull request May 16, 2022
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.

None yet

2 participants