-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
templates: migrate Research Group template from v1 to v2 blocks #2914
Conversation
✅ Deploy Preview for hugo-portfolio-theme canceled.
|
👷 Deploy Preview for markdown-slides processing.
|
✅ Deploy Preview for markdown-slides canceled.
|
Added the requested changes. In addition, I also substituted Also, I updated the first comment with the new parameters for the As soon as the PR will be merged, I'll send another one to fix the |
@Agos95 please resolve the merge conflicts (contact.html) |
Solved! |
{{/* Sort */}} | ||
{{ $sort_by := $block.content.sort_by | default "last_name" }} | ||
{{ if not ( in "last_name first_name title" $sort_by ) }} {{ $sort_by = "last_name" }} {{ end }} | ||
{{ $sort_by = printf ".Params.%s" $sort_by }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we prefix .Params.
to the sort_by
automatically, we should probably do the same for any other uses of the sort_by option in Wowchemy, such as in the Collections (and Portfolio?) blocks, otherwise different behaviours will be very confusing to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I also remove the automatic .Params.
prefix to align this block to the others.
Although, I think maybe it could be nice to have an autoprefix just if last_name
, first_name
or title
are passed as parameters, since these are the most common options to sort the people.
It could be something like:
{{ if in "last_name first_name title" $sort_by }} {{ $sort_by = printf ".Params.%s" $sort_by }} {{ end }}
What do you think? Otherwise, I'll just remove it, no problem!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the first letter of the sort_by param is capitalised then it's a built-in Hugo param right, otherwise it's a parameter which requires prefixing with Params.
?
I have also observed that users find this Hugo behaviour very confusing and prone to error.
So I suggest we check if first letter of sort_by is capitalised, if it is then pass it as it is, otherwise prefix it. If this works successfully, then let's apply this logic to all instances where sort_by is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to think about this. In principle, I think nothing stops me to create a custom parameter (eg. LastName
instead of last_name
), so I don't know if this logic can actually be applied.
If you agree, we can close this PR (which I'll immediately follow with a new one to fix the Wowchemy version in go.mod
) as it is, so that users need to input .Params.last_name
to sort authors by their last name.
Then, I'll open another issue to discuss and investigate about your proposed solution to discover built-in or custom parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Agos95 the Wowchemy parameters are all standardised to lowercase-underscore convention, so that is fine, we can go ahead and make things easier for the user based on my above comment at #2914 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I added the function in the wowchemy-core
module, since it is independent on bootstrap, and I tested it locally.
I also add it in people
, collection
and portfolio
blocks as comments. This is due to the fact that the version of wowchemy-core
must be updated (in modules/wowchemy/go.mod
) in order to detect the new function, and I don't have any idea on what I should do.
Maybe the best thing is to merge this PR, create a new tag/version for wowchemy-core
, and then I can follow with another PR to fix the versions. But I wait for your instructions, since I don't have any experience on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2914 (comment)
In order to merge this PR without splitting it into separate PRs for each module change, we will go ahead with the latter suggestion you made.
@Agos95 You have attempted to modify 3 Go Modules which depend on each other in a single PR. However, Hugo needs us specify in each module's go.mod which versions of modules it depends on and we cannot retrieve the version (Git commit ref) until we have merged the PR. So in future, we should generally submit a separate PR for each module when improvements are made to multiple inherited modules. To help facilitate merging this, please rebase the branch on the latest "main", uncomment the commented code (it's bad practice for us to commit commented code like Then once we merge, please submit an update as soon as you can to fix each module's go.mod to require a specific version. Also, feel free to feedback to Hugo team if you have suggestions on how they can improve the module system to make it easier for contributors. |
Purpose
Content of this PR:
<p></p>
tag, to preserve the correct spacing between the text and the contact formDocumentation
Need to create two entries in the blocks documentation for the v2 version of Slider and People.
Updated versions of the code blocks in the docs are: