-
Notifications
You must be signed in to change notification settings - Fork 4
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
[NAE-1907] - Prompt user before leaving/reloading site when data does not need to be saved #208
base: release/6.4.0
Are you sure you want to change the base?
Conversation
…ot need to be saved - prompt user with dialog when leaving/reloading site and focus is still in input of field (text/number/i18n) - if user decide to stay on site, blur input field so data is saved
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
SonarCloud Quality Gate failed. 0 Bugs 43.3% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
@@ -8,7 +8,9 @@ | |||
[placeholder]="numberField.placeholder" | |||
[required]="numberField.behavior.required" | |||
(focusout)="onFocusOut($event)" | |||
(focusin)="onFocusIn()"> | |||
(focusin)="onFocusIn()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working correctly ? because there are assigned two different functions two times, isn't it better to assigned it like (focusin)="onFocusIn();numberField.setFocus()" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to use dedicated function for multiple functions call on output event.
@@ -62,6 +62,8 @@ export class EasymdeWrapperComponent implements OnDestroy, AfterViewInit, Contro | |||
this._easyMDE = new EasyMDE(this.options); | |||
this._easyMDE.value(this.textAreaField.value); | |||
this._easyMDE.codemirror.on('change', this._onChange); | |||
this._easyMDE.codemirror.on('focus', () => this.textAreaField.setFocus()); | |||
this._easyMDE.codemirror.on('blur', () => this.textAreaField.unsetFocus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was tested ? is it working ?
@@ -8,7 +8,9 @@ | |||
[placeholder]="numberField.placeholder" | |||
[required]="numberField.behavior.required" | |||
(focusout)="onFocusOut($event)" | |||
(focusin)="onFocusIn()"> | |||
(focusin)="onFocusIn()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to use dedicated function for multiple functions call on output event.
@@ -50,7 +50,8 @@ | |||
[placeholder]="textI18nField.placeholder ? textI18nField.placeholder : ''" | |||
[required]="textI18nField.behavior.required" | |||
[(ngModel)]="currentValue[selectedLanguage]" | |||
(blur)="setSelectedValue()"> | |||
(focusout)="setSelectedValue(); textI18nField.unsetFocus()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to use dedicated function for multiple functions call on output event.
# Conflicts: # projects/nae-example-app/src/app/app.module.ts # projects/netgrif-components-core/src/lib/data-fields/models/abstract-data-field.ts # projects/netgrif-components-core/src/lib/data-fields/text-field/textarea-field/abstract-textarea-field.component.ts # projects/netgrif-components/src/lib/data-fields/number-field/number-currency-field/number-currency-field.component.html # projects/netgrif-components/src/lib/data-fields/number-field/number-default-field/number-default-field.component.html # projects/netgrif-components/src/lib/data-fields/text-field/html-textarea-field/html-textarea-field.component.html # projects/netgrif-components/src/lib/data-fields/text-field/simple-text-field/simple-text-field.component.html # projects/netgrif-components/src/lib/data-fields/text-field/text-field.component.ts # projects/netgrif-components/src/lib/data-fields/text-field/textarea-field/textarea-field.component.html
- updated to 6.4.0
|
||
constructor(@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<T>) { | ||
constructor(@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<T>, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I know, setting default value for an @Optional
injector does not work as desired.
null
is a falsy value, so it still works as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if there is no such injection token provided, it will be considered as null, which will indicate falsy value, but I think, the default value is used because of without that it would be needed to add the @Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean
to constructors of every subclass of AbstractBaseDataFieldComponent
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<I18nField>) { | ||
super(dataFieldPortalData); | ||
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<I18nField>, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the default value (even if it doesn't work) should be provided only at the class that introduces the dependency (AbstractBaseDataFieldComponent
in this case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
constructor(@Optional() @Inject(NAE_INFORM_ABOUT_INVALID_DATA) informAboutInvalidData: boolean | null) { | ||
super(informAboutInvalidData); | ||
constructor(@Optional() @Inject(NAE_INFORM_ABOUT_INVALID_DATA) informAboutInvalidData: boolean | null, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value in inheritance chain
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<I18nField>) { | ||
super(languageIconsService, _translateService, dataFieldPortalData); | ||
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<I18nField>, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value in inheritance chain
} | ||
|
||
protected constructor(@Optional() @Inject(NAE_INFORM_ABOUT_INVALID_DATA) protected _informAboutInvalidData: boolean | null, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) protected _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value doesn't work
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<TextAreaField>) { | ||
super(_translate, dataFieldPortalData); | ||
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<TextAreaField>, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value in inheritance chain
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<TextField>) { | ||
super(_translate, dataFieldPortalData); | ||
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<TextField>, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value in inheritance chain
textFieldComponentEnum = TextFieldComponentEnum; | ||
|
||
constructor(@Optional() @Inject(NAE_INFORM_ABOUT_INVALID_DATA) informAboutInvalidData: boolean | null, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) saveDataInform: boolean | null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value in inheritance chain
@@ -7,11 +7,14 @@ | |||
#textArea | |||
[placeholder]="dataField.placeholder" | |||
[required]="dataField.behavior.required" | |||
[formControl]="formControlRef" | |||
[(ngModel)]="dataField.value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? A lot of functionality (e.g. enabling/disabling) depends on the FormControl
as far as I know.
I notice the hidden <input>
on line 17. Does it work, when for example disabling the hidden field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camperko I think you can answer this.
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<TextAreaField>) { | ||
super(_translate, _ngZone, dataFieldPortalData); | ||
@Optional() @Inject(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<TextAreaField>, | ||
@Optional() @Inject(NAE_SAVE_DATA_INFORM) _saveDataInform: boolean | null = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default value in inheritance chain
- updated according to PR
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
Description
Implements NAE-1907
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually
Test Configuration
<Please describe configuration for tests to run if applicable, like program parameters, host OS, VM configuration etc. You can use >
Checklist: