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

English showcase #1273

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

English showcase #1273

wants to merge 6 commits into from

Conversation

almet
Copy link
Member

@almet almet commented Nov 23, 2023

Some work to integrate what @TheodoreCouz has done in #1271

TheodoreCouz and others added 5 commits November 14, 2023 11:02
- Put the images in a language folder ("en", "fr"), which will make it
  easier to add orther languages later on.
- Resize the images to fit the already existing ones.
- Add a `display_showcase` parameter to the
  `list_bills` and `home` views.
@zorun
Copy link
Collaborator

zorun commented Nov 23, 2023

Hey, thanks for your work @TheodoreCouz !

Here is a review on the current version (not sure what comes from you or from @almet ):

  • I'm not sure it makes sense to have a configuration variable for this. What happens if people add "sv" to this variable? There's no images in Swedish so it won't work.
  • I would suggest always displaying the English version when we don't have images in the current language/ That would even encourage people to translate the comics (but of course if we end up with 1 MB of images for each language, then it becomes a packaging issue)
  • minor nitpick: there are French labels such as "Fermer", "Suivant", "Précédent"
  • before merging, don't forget to squash the huge commits with the mistakenly commited libs

@almet
Copy link
Member Author

almet commented Nov 23, 2023

Thanks for the feedback.

I'm not sure it makes sense to have a configuration variable for this. What happens if people add "sv" to this variable? There's no images in Swedish so it won't work.

The setting was actually intended, so that if your language is in there, then the button is displayed, and fallsback to english if your language isn't there.

But we could also always display it.

minor nitpick: there are French labels such as "Fermer", "Suivant", "Précédent"

Ah, I missed these ones. I'll add them to the translation.

before merging, don't forget to squash the huge commits with the mistakenly commited libs

:-)

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