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

Journeys in environment configuration #242

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

arrilight
Copy link

This PR proposes a file-structure change for the apps: journey bundles will now live in shared libraries instead of inside of the app. This way we can dynamically change journey-list based on the configuration of the environment, having multiple apps with different journeys inside of one.

Copy link

@unlimitedjest unlimitedjest left a comment

Choose a reason for hiding this comment

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

All in all I think this is a solid approach, and I'm excited for the changes!

Choose a reason for hiding this comment

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

We'll need some kind of support to define and render sub-menu items

Choose a reason for hiding this comment

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

The file name bundle is a bit confusing, especially with the more specific file name including bundle. Maybe including route in the name somewhere would help clarify at a glance?

Choose a reason for hiding this comment

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

Picking transactions as an example, but my comment would apply in general:

It could be useful to separate customer defined journey configuration options from options set in the product.

For example, I'd have a file journey-config-overrides that exports

export const journeyConfigOverrides: Partial<TransactionsJourneyConfiguration> = {
   showPendingOnTop: true,
   showCheckImages: true,
   accountInfoProperties: { ...goes here }
}

that would then be consumed by the generated journey bundle file here:

{
      provide: TransactionsJourneyConfiguration,
      deps: [EnvironmentService],
      useFactory: (envService: EnvironmentService) =>
        ({
          pageSize: 10,
          slimMode: envService.environment?.common.designSlimMode,
          ...journeyConfigOverrides
        } as TransactionsJourneyConfiguration),
}

Basically, during an upgrade, having this separation would 1) ensure we preserve journey configuration options set during development and 2) make it more transparent which values were set OOTB and which were added by the customer.

import { Navigation, triplets } from '@backbase-gsa/shared';

export const ACH_POSITIVE_PAY_JOURNEY_NAVIGATION: Navigation = {
name: $localize`Ach Positive pay link@@main.ach-positive-pay.link.text:ACH Positive Pay`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: $localize`Ach Positive pay link@@main.ach-positive-pay.link.text:ACH Positive Pay`,
name: $localize`:Ach Positive pay link@@main.ach-positive-pay.link.text:ACH Positive Pay`,

import { Navigation } from '@backbase-gsa/shared';

export const CUSTOM_PAYMENT_JOURNEY_NAVIGATION: Navigation = {
name: $localize`Make a Payment Link@@main.make-a-payment.link.text:Make a payment`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: $localize`Make a Payment Link@@main.make-a-payment.link.text:Make a payment`,
name: $localize`:Make a Payment Link@@main.make-a-payment.link.text:Make a payment`,

import { triplets, Navigation } from '@backbase-gsa/shared';

export const TRANSACTIONS_JOURNEY_NAVIGATION: Navigation = {
name: $localize`transactions list link@@main.transactions.link.text:Transactions List`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: $localize`transactions list link@@main.transactions.link.text:Transactions List`,
name: $localize`:transactions list link@@main.transactions.link.text:Transactions List`,

Comment on lines +29 to +33
useFactory: (envService: EnvironmentService) =>
({
pageSize: 10,
slimMode: envService.environment?.common.designSlimMode,
} as TransactionsJourneyConfiguration),
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to avoid casting wherever possible as it can mask some underlying type errors. Here, for example, slimMode is defined as just boolean in the TransactionsJourneyConfiguration type but the object being cast as that type could actually have boolean | undefined as the type of slimMode.

Refactoring to have an explicit return type on the factory exposes this as a compile error:

Suggested change
useFactory: (envService: EnvironmentService) =>
({
pageSize: 10,
slimMode: envService.environment?.common.designSlimMode,
} as TransactionsJourneyConfiguration),
useFactory: (envService: EnvironmentService): TransactionsJourneyConfiguration =>
({
pageSize: 10,
slimMode: envService.environment?.common.designSlimMode,
}),

Copy link
Contributor

Choose a reason for hiding this comment

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

(I appreciate this isn't part of the point of the PR - there's a lot of stuff in the GSA that could do with cleaning up.)

</span>
</a>
</li>
} @else {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of duplication of html structure here, just because *bbIfEntitlements will display nothing when given undefined. Maybe we can avoid this using an ng-template...

      @for (journey of journeys; track journey) {

        <ng-template #journeyLinkTemplate>
          <li class="bb-layout__horizontal-nav-item">
             <!-- ...etc -->
          </li>
        </ng-template>

        @if(journey.permissions) {
          <ng-container *bbIfEntitlements="journey.permissions">
            <ng-container *ngTemplateOutlet="journeyLinkTemplate" />
          </ng-container>
        } @else {
          <ng-container *ngTemplateOutlet="journeyLinkTemplate" />
        }

Maybe even better would be to speak with the web-foundation team to ask if they can change the *bbIfEntitlements directive so that if given a certain value, say '*', then it automatically shows the content without checking any entitlements. Then we could just do this:

      <li
        *bbIfEntitlements="journey.permissions ?? '*'"
        class="bb-layout__horizontal-nav-item"
      >
        <!-- ... etc -->
      </li>

@tcorral
Copy link
Collaborator

tcorral commented Jan 9, 2025

Good morning, @daiscog , @unlimitedjest .

What is the purpose and the status of this PR?

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.

4 participants