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

frontend: refactor layout #107

Merged
merged 1 commit into from
Oct 11, 2018
Merged

frontend: refactor layout #107

merged 1 commit into from
Oct 11, 2018

Conversation

jma
Copy link
Contributor

@jma jma commented Oct 9, 2018

  • BETTER: css and js bundles
  • BETTER: include reroils-record-editor in the app
  • BETTER: editor views based on Invenio-Admin
  • NEW: professional sidebar
  • BETTER: refactor header layout
  • BETTER: reduce the number of search jinja templates

Co-Authored-by: Johnny Mariéthoz [email protected]
Co-Authored-by: Igor Milhit [email protected]

Note:

  • draft version of the prof. sidebar

Tests:

  • pipenv run pip uninstall reroils-record-editor and pipenv run pip install -e .[all]
  • ./scripts/bootstrap, ./scripts/setup, ./scripts/server or adapt your own
  • url interface: https://localhost:5000

@jma jma requested review from Garfield-fr and rerowep October 9, 2018 13:34
@jma jma force-pushed the header-top-menu branch 2 times, most recently from 45a9321 to e82cfe7 Compare October 9, 2018 15:41
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.

Some links have to be updated:

  • from the patrons brief and detailed view to the circulation module, and when possible use a url_for.

# waive the privileges and immunities granted to it by virtue of its status
# as an Intergovernmental Organization or submit itself to any jurisdiction.

"""Utilities for reroils-record-editor."""

Choose a reason for hiding this comment

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

Change Docstring.

@Garfield-fr
Copy link
Contributor

Header menu: Blue on blue is not good. we can't see well. Change with white color.

@Garfield-fr
Copy link
Contributor

Header: The logo is smaller than before

@Garfield-fr
Copy link
Contributor

Header menu: The "menu" menu is not the right term, because it contains languages. Change with "Languages" or other and extract help to another place.

@Garfield-fr
Copy link
Contributor

Homepage: The white writing on the image has changed. Characters too small.

@Garfield-fr
Copy link
Contributor

Layout: With the admin menu, the whole presentation is shifted to the right.

@Garfield-fr
Copy link
Contributor

Homepage: The red banner with the development text is too small.

# -*- coding: utf-8 -*-
#
# This file is part of Invenio.
# Copyright (C) 2017 RERO.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright (C) 2018 RERO.

Copy link
Contributor

@rerowep rerowep left a comment

Choose a reason for hiding this comment

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

screenshot 2018-10-10 at 08 19 28

Warnings display to improve

@@ -1,9 +1,9 @@
<ul class="re-list">
<li ng-controller="recordController" ng-init="rec=record" ng-repeat="record in vm.invenioSearchResults.hits.hits track by record.id">
<div class="col-xs-1">
<div class="col-xs-2">
<icon-thumbnail class="thumb-brief" identifiers="{{ record.metadata.identifiers }}" type="{{ record.metadata.type }}" config="{{ config }}"></icon-thumbnail>
Copy link
Contributor

Choose a reason for hiding this comment

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

record.metadata.identifiers not defined for new records

Choose a reason for hiding this comment

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

That was a bug from 2eb9bde

Choose a reason for hiding this comment

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

@rerowep But does this issue affect the brief view rendering?

# waive the privileges and immunities granted to it by virtue of its status
# as an Intergovernmental Organization or submit itself to any jurisdiction.

"""reroils record editor."""
Copy link
Contributor

Choose a reason for hiding this comment

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

remove reroils term. Change with "Record editor."

# -*- coding: utf-8 -*-
#
# This file is part of Invenio.
# Copyright (C) 2017 RERO.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright (C) 2018 RERO.

# waive the privileges and immunities granted to it by virtue of its status
# as an Intergovernmental Organization or submit itself to any jurisdiction.

"""reroils translation json utils."""
Copy link
Contributor

Choose a reason for hiding this comment

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

remove reroils term. Change with "Translation json utils."

<base href="/items/circulation">
{%- extends 'admin/master.html' %}
{%- block head_meta %}
<base href="/admin/circulation">
Copy link
Contributor

Choose a reason for hiding this comment

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

url_for ?

"""Angular circulation application."""
return render_template('rero_ils/circulation_ui.html')


def item_view_method(pid, record, template=None, **kwargs):
r"""Display default view.
Copy link
Contributor

Choose a reason for hiding this comment

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

r before " ?

Choose a reason for hiding this comment

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

@Garfield-fr It seems to be a way to allow \ in Docstrings.

@@ -0,0 +1,9 @@
{# -*- coding: utf-8 -*-

This file is part of Invenio.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is part of RERO ILS.

{# -*- coding: utf-8 -*-

This file is part of Invenio.
Copyright (C) 2015-2018 CERN.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright (C) 2018 RERO.

{% extends 'invenio_theme/page.html' %}
{# -*- coding: utf-8 -*-

This file is part of Invenio.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is part of RERO ILS.

@@ -0,0 +1,69 @@
{# -*- coding: utf-8 -*-

This file is part of Invenio.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is part of RERO ILS.

{# -*- coding: utf-8 -*-

This file is part of Invenio.
Copyright (C) 2015-2018 CERN.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright (C) 2018 RERO.

@jma jma force-pushed the header-top-menu branch from e82cfe7 to 35b323b Compare October 11, 2018 06:14
"""Evaluate if the resource can be created."""
adm = current_app.extensions['invenio-admin'].admin

def get_admin_view(record_type, menus):
Copy link
Contributor

Choose a reason for hiding this comment

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

def ident isn't good

adm = current_app.extensions['invenio-admin'].admin

def get_admin_view(record_type, menus):
"""."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing docstring



def admin_permission_factory(admin_view):
"""."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing docstring

@rerowep
Copy link
Contributor

rerowep commented Oct 11, 2018

Needs max width or responsive image width.

@jma jma force-pushed the header-top-menu branch from 35b323b to 970a4ea Compare October 11, 2018 14:48
* BETTER: css and js bundles
* BETTER: include reroils-record-editor in the app
* BETTER: editor views based on Invenio-Admin
* NEW: professional sidebar
* BETTER: refactor header layout
* BETTER: reduce the number of search jinja templates
* FIX: closes rero#109

Co-Authored-by: Johnny Mariéthoz <[email protected]>
Co-Authored-by: Igor Milhit <[email protected]>
Signed-off-by: Igor Milhit <[email protected]>
@jma jma force-pushed the header-top-menu branch from 970a4ea to 03bcd7a Compare October 11, 2018 16:26
@jma jma merged commit 0fd2b8a into rero:master Oct 11, 2018
@jma jma deleted the header-top-menu branch November 28, 2018 06:02
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.

4 participants