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

holdings: improve editor preview rendering #413

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

jma
Copy link
Contributor

@jma jma commented Nov 4, 2020

  • Reduces the number of http calls when the form is modified.

Co-Authored-by: Johnny Mariéthoz Johnny.Mariethoz@rero.ch

Why are you opening this PR?

  • To make the holding editor more responsive.

How to test?

  • Go to the holding editor for the serials. This prediction preview should not make too much backend http calls.

Code review check list

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

// only if the patterns changed
distinctUntilChanged((a, b) => JSON.stringify(a.patterns) === JSON.stringify(b.patterns)),
// cancel previous pending requests
switchMap(modelValue => this._editorService.getHoldingPatternPreview(
Copy link
Contributor

@zannkukai zannkukai Nov 4, 2020

Choose a reason for hiding this comment

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

switchMap seems great and powerful !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exact! I put some comment on each, because I'm pretty sure that I will forget the meaning of each in few days...

Copy link
Contributor

@iGormilhit iGormilhit left a comment

Choose a reason for hiding this comment

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

Commit message: emphasis the fact that this is for the holdings editor. For the title, I would simplify: holdings: improve editor preview rendering or something like it.

@iGormilhit iGormilhit added this to the v0.14.0 milestone Nov 5, 2020
* Reduces the number of http calls when the form is modified.

Co-Authored-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>
@jma jma force-pushed the maj-fix-multiple-http-holdings branch from 8c7ccf3 to 058d1d1 Compare November 5, 2020 09:38
@jma
Copy link
Contributor Author

jma commented Nov 5, 2020

holdings: improve editor preview rendering

Commit message: emphasis the fact that this is for the holdings editor. For the title, I would simplify: holdings: improve editor preview rendering or something like it.

You are completely right. Thanks to read all our commit message and in particular my poor message quality!

@jma jma requested a review from iGormilhit November 5, 2020 09:39
@iGormilhit iGormilhit changed the title holdings: avoid multiple http calls for preview holdings: improve editor preview rendering Nov 5, 2020
Copy link
Contributor

@iGormilhit iGormilhit left a comment

Choose a reason for hiding this comment

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

Commit message approved.

@@ -43,34 +45,59 @@ export class HoldingEditorComponent {
/** Current error message from the backend during the serial preview example if exists */
serialPreviewError = null;

/** */

Choose a reason for hiding this comment

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

Comment is missing

@iGormilhit iGormilhit merged commit 1acd323 into rero:dev Nov 12, 2020
@jma jma deleted the maj-fix-multiple-http-holdings branch February 11, 2021 06:53
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