Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
templates: migrate Research Group template from v1 to v2 blocks #2914
Changes from 11 commits
d715d64
3b41772
ae69c98
43aa430
bc368f4
cb407f2
2cbdecd
6d828c1
8267e13
07ff821
ad1dd0a
23de333
d3c4752
67731aa
19b3dab
6ad754a
0ecaf4e
4356626
412907b
7a2d5f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 thesort_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
ortitle
are passed as parameters, since these are the most common options to sort the people.It could be something like:
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 oflast_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
andportfolio
blocks as comments. This is due to the fact that the version ofwowchemy-core
must be updated (inmodules/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.
This file was deleted.
This file was deleted.