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

💄 Adjust metadata output #91

Merged
merged 8 commits into from
Jan 24, 2022
Merged

💄 Adjust metadata output #91

merged 8 commits into from
Jan 24, 2022

Conversation

cbelena
Copy link
Contributor

@cbelena cbelena commented Jan 23, 2022

Hi @jpanther! Per discussion #88:

I have submitted two commits:

  1. Add i18n 'Updated' strings: I have added the article.updated string to the different languages available, making possible the translations to the last updated date in article's metadata. I've also updated the missing spanish translations of "word" and "words".
  2. Adjust metadata output: I have created a new partial called date-updated.html which adds the 'Updated' localized string and then calls the date.html partial. The new simplified date.html also solves a little problem where before it displayed both the date and the updated date on pagination. Then, in article-meta.html, instead of showing the last updated date in parentheses, I have treated it like another meta parameter.

This is my first ever pull request, I'm sorry if I did something wrong 😅

@netlify
Copy link

netlify bot commented Jan 23, 2022

✔️ Deploy Preview for hugo-congo ready!
Built without sensitive environment variables

🔨 Explore the source changes: 6313f69

🔍 Inspect the deploy log: https://app.netlify.com/sites/hugo-congo/deploys/61ede71fcd387a00074123fb

😎 Browse the preview: https://deploy-preview-91--hugo-congo.netlify.app

@github-actions github-actions bot added the i18n Issue with i18n translations label Jan 23, 2022
@cbelena
Copy link
Contributor Author

cbelena commented Jan 23, 2022

Oh, I have not included my CSS fix to wrap the text around the metadata because I'm not sure if that's the best way to do it 😅

@jpanther
Copy link
Owner

Thanks for the PR @cbelena. A very good first pull request! I'm happy with the changes however my only concern is that using the time.Format function requires a bump in the minimum Hugo version which prevents this from being a patch release. Can we leave this with Date.Format for now and it will pick up the new function in v2 of the theme?

And yes don't worry about the CSS. I'll add a commit that addresses that part for you. It requires the Tailwind CSS to be recompiled.

@jpanther jpanther added the enhancement New feature or request label Jan 23, 2022
@jpanther
Copy link
Owner

I'm also now wondering if we should pass the date into i18n as a variable too. I'm not sure but there may be some languages that require the format "{Date} Updated" rather than "Updated {Date}".

@cbelena
Copy link
Contributor Author

cbelena commented Jan 23, 2022

Thanks for the PR @cbelena. A very good first pull request! I'm happy with the changes however my only concern is that using the time.Format function requires a bump in the minimum Hugo version which prevents this from being a patch release. Can we leave this with Date.Format for now and it will pick up the new function in v2 of the theme?

Sure! I reverted to Date.Format instead of time.Format.

And yes don't worry about the CSS. I'll add a commit that addresses that part for you. It requires the Tailwind CSS to be recompiled.

Wow, that was easy!! I have to learn more CSS! 😅😅 Thanks!

I'm also now wondering if we should pass the date into i18n as a variable too. I'm not sure but there may be some languages that require the format "{Date} Updated" rather than "Updated {Date}".

I have left it the same for now because it has a colon between the 'Update' and the date, like: Updated: Jan 24, 2022. But I'm not sure either...

PS: Oops, I forgot to add the globe emoji to the commit that updates the TR translation, sorry!

@jpanther
Copy link
Owner

The CSS turned out to be as simple as adding flex-wrap to the div so this was an easy fix. I think the rest looks good though so I'll merge this in now.

@jpanther jpanther merged commit 9fc0a44 into jpanther:dev Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request i18n Issue with i18n translations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants