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

assets: move to webpack #1537

Merged
merged 1 commit into from
Dec 7, 2020
Merged

assets: move to webpack #1537

merged 1 commit into from
Dec 7, 2020

Conversation

jma
Copy link
Contributor

@jma jma commented Dec 2, 2020

  • Fixes docker file to use a custom rero-ils-ui file.
  • Removes npm utils from the bootstrap script.
  • Removes angularjs translations extraction.
  • Moves all theme related files into a specific directory.
  • Removes all bundles files.
  • Removes useless INVENIO_SEARCH_UI configuration variables.
  • Includes the angular application with a custom manner: it does not
    make sense to use invenio-assets for an already optimized bundle.
  • Removes js to store the last visited html tab, closes The tab displayed when opening a detailed view seems to be random. #1394.
  • Removes angularjs dependency.
  • Use simple code to generate thumbnails in the document detailed view.
  • Reduces the docker image size by cleaning several cache files.
  • Closes Static files on production delivers more file than expected ie. package-lock.json #713.

Why are you opening this PR?

  • To remove legacy code.

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?

@jma jma changed the title Maj webpack assets: move to webpack Dec 2, 2020
@jma jma force-pushed the maj-webpack branch 11 times, most recently from b4aa9a2 to c60c897 Compare December 3, 2020 07:25
@jma jma marked this pull request as ready for review December 3, 2020 07:25
@jma jma force-pushed the maj-webpack branch 2 times, most recently from ffec4a4 to 3ef3d6f Compare December 3, 2020 12:18
@jma jma force-pushed the maj-webpack branch 2 times, most recently from 6cee8b0 to 40fcec9 Compare December 3, 2020 14:02
Comment on lines 47 to 56
to_return = '<script {tags} src="{value}"></script>'.format(
value=value,
tags=tags
)
if _type == 'css':
to_return = '<link {tags} href="{value}" rel="stylesheet">'.format(
value=value,
tags=tags
)
return to_return
Copy link
Contributor

Choose a reason for hiding this comment

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

if _type == 'css':
    return '<link {tags} href="{value}" rel="stylesheet">'.format(
        value=value,
        tags=tags
    )
return '<script {tags} src="{value}"></script>'.format(
    value=value,
    tags=tags
)

It seems "dummy" to intitialize a variable and override it just after

<figure class="mb-0 thumb">
<img class="img-responsive border border-light"
src="{{record | get_cover_art}}"
onerror="this.src='{{url_for("static", filename="images/icon_%s.png" % record.type) }}'" >
Copy link
Contributor

Choose a reason for hiding this comment

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

👍😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was your idea! Thanks.

url, headers={'referer': flask_request.host_url})

if response.status_code != 200:
current_app.logger.warning(
Copy link
Contributor

Choose a reason for hiding this comment

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

It will log a looooooooot of message isn't it ? Maybe warning level isn't the best for this message ? info/debug ?

Comment on lines +47 to +50
var sPageURL = window.location.search.substring(1),
sURLVariables = sPageURL.split('&'),
sParameterName,
i;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmm.... strange construction, I would trust you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too, I found this code on the web and seems work.

Copy link

@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.

@@ -197,13 +182,14 @@ def _(x):
#: Template for error pages.
THEME_ERROR_TEMPLATE = 'rero_ils/page_error.html'
#: RERO-ils search endpoint (i.e /search/documents)
RERO_ILS_THEME_SEARCH_ENDPOINT = '/search/documents'
# RERO_ILS_THEME_SEARCH_ENDPOINT = '/search/documents'

Choose a reason for hiding this comment

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

Do you actually want to comment this, or do you want to remove it?

@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
#
# RERO ILS
# Copyright (C) 2020 RERO
# Copyright (C) 2019 RERO

Choose a reason for hiding this comment

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

Why are you going in the past? I know, 2020 is an awful year, but still.

/*

RERO ILS
Copyright (C) 2019 RERO

Choose a reason for hiding this comment

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

Suggested change
Copyright (C) 2019 RERO
Copyright (C) 2020 RERO

/*

RERO ILS
Copyright (C) 2019 RERO

Choose a reason for hiding this comment

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

Suggested change
Copyright (C) 2019 RERO
Copyright (C) 2020 RERO

@@ -1,7 +1,7 @@
/*

RERO ILS
Copyright (C) 2020 RERO
Copyright (C) 2019 RERO

Choose a reason for hiding this comment

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

Suggested change
Copyright (C) 2019 RERO
Copyright (C) 2020 RERO

@iGormilhit iGormilhit added the dev: dependencies Related to a dependency that needs to be fixed, or pinned, or changed label Dec 3, 2020
@iGormilhit iGormilhit added this to the v0.15.0 milestone Dec 3, 2020
@zannkukai
Copy link
Contributor

icon for ill request form is missing in the user "tool" menu

@lauren-d
Copy link
Contributor

lauren-d commented Dec 3, 2020

I'm not sure that is linked to angular 11.
There is a little bit strange "blink effect" when we use facets on any search view.
It seems the search is processed twice:

Capture d’écran 2020-12-03 à 20 45 45

@AoNoOokami
Copy link
Contributor

AoNoOokami commented Dec 4, 2020

I'm not sure that is linked to angular 11.
There is a little bit strange "blink effect" when we use facets on any search view.
It seems the search is processed twice...

There is an issue about that: #1179

@AoNoOokami
Copy link
Contributor

AoNoOokami commented Dec 4, 2020

After the merge of previous PR to use Angular 11, brief and detailed views take too much space in width. There is a class main-content used in ng-core, to which a style is applied in rero-ils ui that should manage that. It seems ineffective now. Check that behaviour too in your PR please.
Context: #1514 and rero/rero-ils-ui#440 after rebase on rero/dev with rero-ils-ui integrated to rero-ils (localhost:5000).

@AoNoOokami
Copy link
Contributor

AoNoOokami commented Dec 4, 2020

I saw that it is not possible to do a request in the pro interface. The modal to enter patron barcode and choose pickup is not displayed. That makes several cypress tests fail.
Context: #1514 and rero/rero-ils-ui#440 after rebase on rero/dev with rero-ils-ui integrated to rero-ils (localhost:5000).

* Removes npm utils from the bootstrap script.
* Removes angularjs translations extraction.
* Moves all theme related files into a specific directory.
* Removes all bundles files.
* Removes useless INVENIO_SEARCH_UI configuration variables.
* Includes the angular application with a custom manner: it does not
  make sense to use `invenio-assets` for an already optimized bundle.
* Removes js to store the last visited html tab, closes rero#1394.
* Removes angularjs dependency.
* Use simple code to generate thumbnails in the document detailed view.
* Reduces the docker image size by cleaning several cache files.
* Closes rero#713.

Signed-off-by: Johnny Mariéthoz <Johnny.Mariethoz@rero.ch>
@jma
Copy link
Contributor Author

jma commented Dec 7, 2020

I'm not sure that is linked to angular 11.
There is a little bit strange "blink effect" when we use facets on any search view.
It seems the search is processed twice...

There is an issue about that: #1179

I don't think it is related to this issue. This happens on the search list view, not in the circulation.

@jma
Copy link
Contributor Author

jma commented Dec 7, 2020

After the merge of previous PR to use Angular 11, brief and detailed views take too much space in width. There is a class main-content used in ng-core, to which a style is applied in rero-ils ui that should manage that. It seems ineffective now. Check that behaviour too in your PR please.
Context: #1514 and rero/rero-ils-ui#440 after rebase on rero/dev with rero-ils-ui integrated to rero-ils (localhost:5000).

Unable to reproduce.

@jma
Copy link
Contributor Author

jma commented Dec 7, 2020

I saw that it is not possible to do a request in the pro interface. The modal to enter patron barcode and choose pickup is not displayed. That makes several cypress tests fail.
Context: #1514 and rero/rero-ils-ui#440 after rebase on rero/dev with rero-ils-ui integrated to rero-ils (localhost:5000).

unable to reproduce

@jma jma merged commit 02e514e into rero:dev Dec 7, 2020
@jma jma deleted the maj-webpack branch January 13, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev: dependencies Related to a dependency that needs to be fixed, or pinned, or changed
Projects
None yet
6 participants