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

#57 für den DHICB-9105 Datenschutzhinweis die Zeta Komponente Login m… #77

Merged
merged 16 commits into from
Oct 31, 2023

Conversation

rojansen
Copy link
Contributor

Die Änderungen an der lokalen ZetaBibliothek aus dem NFRD Projekt wurden in die Zeta Bibliothek übernommen. Es wurde nicht getetstet ob sich der Code bauen und komplilieren lässt

@rojansen rojansen linked an issue Oct 18, 2023 that may be closed by this pull request
@@ -88,6 +91,7 @@ export class AuthLoginComponent {
private readonly dialogService: XcDialogService,
private readonly i18n: I18nService
) {
this.local_i18n = i18n;
Copy link
Contributor

Choose a reason for hiding this comment

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

you already have a local instance of the I18nService through dependency injection. Use with this.i18n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IntelliJ marks a mistake when i directly use this.i18n.
Is it possible that i18n is only accessible in the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as you declare private readonly i18n: I18nService as a constructor parameter, the i18n will be handled as a private member of your class. Maybe IntelliJ is misconfigured for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i found the problem
in the netFactoryComponent.ts
the constructor looks like this
constructor( i18n: I18nService
and not like this
constructor(private readonly i18n: I18nService
and i assume that is the same in the aut login but it isnt

@@ -74,6 +74,9 @@ export class AuthLoginComponent {
}
};

showPrivacyButton = !!environment.zeta.privacyLink;
local_i18n :I18nService;
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax convention would be local_i18n: I18nService;
But you don't need that member, see below.

@@ -127,6 +131,20 @@ export class AuthLoginComponent {
}
}

openPrivacyLink() {
switch(this.local_i18n.language){
Copy link
Contributor

Choose a reason for hiding this comment

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

with the switch, you have to adjust this block for each additional language.
And each project using this feature would have to apply to the convention to only have a lang-parameter for "en" and not for "de", for example. Will be problematic for a non-german company.

I would rather like sth. as
window.open(environment.zeta.privacyLink + '&lang=' + this.i18n.language, '_blank').focus();.

Another, more flexible option is to place a privacyLink: function(lang: string): string into the environment instead of only the string itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this solution doesn't work because this.i18n.language =en-US but the &lang Parameter needs only en
but i added like you suggest a function in the enviroment.ts
so for other companys they extend the switch statement only in the enviroment.ts.
i think the switch statement is neccessary because the this.i18n.languag Parameter and the &lang Parameter doesn t match
and telekom have only links for language german and english

/**
* if the link is set, then the privacy button will be displayed inside the login mask
*/
privacyLink?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

now you won't need this one, anymore, as it would be redundant to the getPrivacyLink function. Only use the above one and merge the respective documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to delete this was my first try, but i run into a problem with this function showPrivacyButton = !!environment.zeta.privacyLink; Iexplain more in a mail

/**
* extend the Privacy Link with the language parameter
*/
getPrivacyLink?(lang: string): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

great! This is what I was expecting.

@@ -74,6 +74,8 @@ export class AuthLoginComponent {
}
};

showPrivacyButton = !!environment.zeta.privacyLink;
Copy link
Contributor

Choose a reason for hiding this comment

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

As you will need this flag below, you should rather rename this bool to what it stands for. E. g. "privacyLinkDefined"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i rename the button see commit

@@ -128,6 +130,12 @@ export class AuthLoginComponent {
}


openPrivacyLink() {
window.open(environment.zeta.getPrivacyLink(this.i18n.language), '_blank').focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

before executing this you should assert that getPrivacyLink is defined. The privacy button may not be able to call this function in that case, but someone else may.
Use the bool flag from above (e. g. privacyLinkDefined).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i add an if statement window.open(environment.zeta.getPrivacyLink(..... will be called when privacyLinkDefined ==true

window.open(environment.zeta.getPrivacyLink(this.i18n.language), '_blank').focus();
}
else{
window.alert("PrivacyLink is not Defined");
Copy link
Contributor

Choose a reason for hiding this comment

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

better use a console.log here. For a severe problem it is okay to interrupt the user's flow with a window.alert, but I think, this is not the case here. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the comment is realize with the last commit windows. alert wird jetzt console.log("PrivacyLink is not Defined"); verwendet

[disabled]="pending"
(click)="login()"
xc-i18n
class="items-left" *ngIf="privacyLinkDefined"
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix indentation

xc-i18n
>privacy-button</xc-button>
<xc-button
class="login-button items-right"
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix indentation

@johannesheucher-gip johannesheucher-gip merged commit 7306e3c into main Oct 31, 2023
4 checks passed
@johannesheucher-gip johannesheucher-gip deleted the 57-add-privacy-button-to-login-mask-2 branch October 31, 2023 16:18
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.

Add Privacy Button to Login Mask
2 participants