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

Novo 7 upgrade new #8

Merged
merged 17 commits into from
Jul 28, 2023
Merged

Novo 7 upgrade new #8

merged 17 commits into from
Jul 28, 2023

Conversation

BantiG
Copy link

@BantiG BantiG commented Jul 18, 2023

No description provided.

import {Configuration} from './settings.types';

@Injectable()
export class SettingsService {

Choose a reason for hiding this comment

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

Remove SettingsService as this is internal. Also remove the "settingsConfig" object from the environment files (all of them).

],
providers: [
AppBridgeService
AppBridgeService,
SettingsService
Copy link
Contributor

Choose a reason for hiding this comment

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

Spoke with Tomas. We do not need the SettingsService included in the Open Source Repo

@@ -48,6 +48,7 @@ export class AppBridgeService {
}

private register() {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

What are we ignoring for this?

Choose a reason for hiding this comment

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

This must be ignoring the type error raised for the register parameter type, which changed slightly - There's a property in the param type named color that used to be a string and now is a AlleyLinkColor (Which is a subset of strings). The color property in the environment.appBridgeConfig object could be cast to AlleyLinkColor, too, which would get rid of the ts error.
cc: @BantiG

"buildOptimizer": false,
"sourceMap": true,
"optimization": false,
"namedChunks": true
},
"configurations": {
Copy link
Member

Choose a reason for hiding this comment

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

Can't comment on the right line because it wasn't changed...but under the staging/local configurations, I would recommend adding
"sourceMap": true, "optimization": false,
at the same level as the fileReplacements property. From what I've seen this allows you to get the source maps and like properly debug

angular.json Outdated
},
"configurations": {
"prod": {
"optimization": true,
"optimization": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to do this for the prod configuration. We want this for the staging and local config. This would just make the production environment slower, which we don't want.

@taswartz taswartz merged commit 5c9493e into bullhorn:master Jul 28, 2023
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.

5 participants