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

libraries: add notification settings #516

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

AoNoOokami
Copy link
Contributor

@AoNoOokami AoNoOokami commented Feb 10, 2021

  • Improves library editor appearance for large screens. It doesn't take
    the full width anymore.
  • Adds tabs in the library detailed view.
  • Adds notifications settings in library custom editor.
  • Displays notification settings in library detail view.

Co-Authored-by: Alicia Zangger alicia.zangger@rero.ch
Co-Authored-by: Johnny Mariéthoz johnny.mariethoz@rero.ch
Co-Authored-by: Alicia Zangger alicia.zangger@rero.ch

Why are you opening this PR?

To implement task 2013 of US1922 https://tree.taiga.io/project/rero21-reroils/us/1922?milestone=282105

Dependencies

How to test?

  • What command should I have to run to test your PR?
  • What should I test through the UI?

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?
  • Cypress tests successful?

@AoNoOokami AoNoOokami added this to the v1.1.0 milestone Feb 10, 2021
@AoNoOokami AoNoOokami self-assigned this Feb 10, 2021
@AoNoOokami AoNoOokami force-pushed the zaa-#2013-printing-email branch 3 times, most recently from 4781212 to bdf7ca9 Compare February 15, 2021 13:59
@AoNoOokami AoNoOokami force-pushed the zaa-#2013-printing-email branch 7 times, most recently from 6fd7b2f to 791322d Compare February 22, 2021 10:08
@AoNoOokami AoNoOokami marked this pull request as ready for review February 22, 2021 15:07
@AoNoOokami AoNoOokami requested review from zannkukai, jma and lauren-d and removed request for jma February 22, 2021 15:07
@AoNoOokami AoNoOokami force-pushed the zaa-#2013-printing-email branch from 791322d to a64e246 Compare February 23, 2021 07:50
public organisationPid: string;

/** List of internal notification types */
internalTypes = ['booking', 'request', 'transit_notice'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be set in a Enum class into class/library.ts file. As ItemStatus, or NoteType

@iGormilhit iGormilhit modified the milestones: v1.1.0, v1.0.2 Mar 1, 2021
@jma jma force-pushed the zaa-#2013-printing-email branch from a64e246 to 960a044 Compare March 11, 2021 14:17
@jma jma self-requested a review March 11, 2021 14:22
@jma jma force-pushed the zaa-#2013-printing-email branch from 960a044 to 6f0b99a Compare March 11, 2021 14:39
@jma jma requested review from zannkukai and Garfield-fr and removed request for lauren-d March 11, 2021 16:32
Copy link
Contributor

@Garfield-fr Garfield-fr left a comment

Choose a reason for hiding this comment

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

With the new version of Angular, it is recommended to type the return of the function.
General: Add documentation on functions

@@ -19,9 +19,14 @@
// required as json properties is not lowerCamelCase

import { WeekDay } from '@angular/common';
import { marker } from '@biesbjerg/ngx-translate-extract-marker';
Copy link
Contributor

Choose a reason for hiding this comment

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

import { marker as _ } from '@biesbjerg/ngx-translate-extract-marker';

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this does not works with the tests.

Comment on lines +26 to +30
export function _(str) {
return marker(str);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Remove this (test extraction)


/** Observable for build event */
private buildEvent = new Subject();

constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing doc string

@@ -29,14 +31,22 @@ export class LibraryFormService {

public form;


private notificationTypes = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

missing doc string


/**
* Build form
*/
build() {
Copy link
Contributor

Choose a reason for hiding this comment

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

build(): void {

@@ -140,6 +238,9 @@ export class LibraryFormService {
get opening_hours() {
return this.form.get('opening_hours') as FormArray;
}
get notification_settings() {
return this.form.get('notification_settings') as FormArray;
}

getValues() { return this.form.value; }
Copy link
Contributor

Choose a reason for hiding this comment

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

add return type

public organisationPid;
public organisationPid: string;

private eventForm: Subscription;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing doc string

public organisationPid;
public organisationPid: string;

private eventForm: Subscription;

constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing doc string

});
}

ngOnDestroy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing doc string and add return type

Comment on lines 22 to 24
<dt class="col-sm-3 offset-sm-2 offset-md-0">
{{ 'Address' | translate }}:
</dt>
Copy link
Contributor

Choose a reason for hiding this comment

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

<dt class="col-sm-3 offset-sm-2 offset-md-0 label-title" translate>
  Address
</dt>

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. I didn't now that this exists. Needs refactor other files in a separate PR.

Copy link
Contributor

@zannkukai zannkukai left a comment

Choose a reason for hiding this comment

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

No other remarks than @Garfield-fr

@jma jma force-pushed the zaa-#2013-printing-email branch from 6f0b99a to e3a8e3b Compare March 15, 2021 07:44
@jma jma requested a review from Garfield-fr March 15, 2021 07:44
@jma jma force-pushed the zaa-#2013-printing-email branch from e3a8e3b to 590ef80 Compare March 15, 2021 08:31
* Improves library editor appearance for large screens. It doesn't take
the full width anymore.
* Adds tabs in the library detailed view.
* Adds notifications settings in library custom editor.
* Displays notification settings in library detail view.

Co-Authored-by: Alicia Zangger <alicia.zangger@rero.ch>
Co-Authored-by: Johnny Mariéthoz <johnny.mariethoz@rero.ch>
@jma jma force-pushed the zaa-#2013-printing-email branch from 590ef80 to e2267fd Compare March 15, 2021 09:14
@jma jma merged commit 65e84b4 into rero:dev Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants