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

global: implement support for tuples in the routes config (part of front page components refactor) #2243

Merged

Conversation

jennur
Copy link
Member

@jennur jennur commented Jun 2, 2023

@jennur jennur force-pushed the frontpage-components-refactor branch 3 times, most recently from 3695456 to 074590e Compare June 23, 2023 14:01
@jennur jennur changed the title frontpage: rewrite frontpage components to python anand jinja frontpage: rewrite communities carousel to python and jinja Jun 23, 2023
@jennur jennur force-pushed the frontpage-components-refactor branch from 074590e to ceefa23 Compare June 23, 2023 14:08
@jennur jennur marked this pull request as ready for review June 23, 2023 14:09
@jennur jennur force-pushed the frontpage-components-refactor branch 2 times, most recently from e742abe to 516e9d2 Compare June 23, 2023 14:34
def index():
"""Frontpage."""
featured_communities = current_communities.service.featured_search(
Copy link
Contributor

Choose a reason for hiding this comment

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

what if an instance will not have feature communities in the home page? This extra search should be avoided in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Plus one on this!

else:
return rule, name, default_view_func

blueprint.add_url_rule(*create_url_rule(rule=routes["index"], name="index", default_view_func=index))
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to use the same way of constructing this for the rest of the routes i.e robots, help_search, help_statistics to be consistent...

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the name there because I couldn't specify which of the arguments were the view_func in add_url_rule through the other function (or is there a way?), so it was failing. But I can remove it from the function parameters and just add None in the return value?

@@ -62,6 +74,8 @@ def init_menu():
#
# Views
#

Copy link
Member

Choose a reason for hiding this comment

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

leftover

@@ -2,6 +2,26 @@
Invenio App RDM Transition Overrides
***********************************************/

#carousel-slides {
Copy link
Member

Choose a reason for hiding this comment

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

there is no carousel anymore in invenio-app-rdm so maybe is better to move these in zenodo-rdm? Or are these changes needed for the existing macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

All the styling is for the macros and/or react component in invenio-communities. zenodo-rdm is just importing the macro from invenio-communities

@inveniosoftware inveniosoftware deleted a comment from zzacharo Jul 4, 2023
@jennur jennur force-pushed the frontpage-components-refactor branch 3 times, most recently from 061a97b to e4699d9 Compare July 4, 2023 13:32
if isinstance(rule, tuple):
path, view_func = rule

if view_func == None:
Copy link
Member

Choose a reason for hiding this comment

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

this can be removed as a tuple should always be used with a view_func

@@ -18,6 +18,18 @@
from invenio_i18n import lazy_gettext as _
from invenio_users_resources.forms import NotificationsForm

# Generate rule from string or tuple
def create_url_rule(rule, default_view_func):
if isinstance(rule, tuple):
Copy link
Member

Choose a reason for hiding this comment

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

you need a docsting here :)

@jennur jennur force-pushed the frontpage-components-refactor branch from e4699d9 to 86eb277 Compare July 4, 2023 13:44
@jennur jennur force-pushed the frontpage-components-refactor branch from 86eb277 to f709372 Compare July 4, 2023 13:51
@zzacharo zzacharo merged commit c6cbde9 into inveniosoftware:master Jul 4, 2023
12 checks passed
@jennur jennur changed the title frontpage: rewrite communities carousel to python and jinja global: implement support for tuples in the routes config (part of front page components refactor) Jul 11, 2023
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.

3 participants