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

New theme 'focus' #1528

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

New theme 'focus' #1528

wants to merge 60 commits into from

Conversation

cintek
Copy link

@cintek cintek commented Sep 26, 2023

As mentioned on the mailing list I would like to contribute a new theme to Moin2.

This commit shows the changes to layout.html.

Screenshot 1: Sidebar and top bar

new_theme_1

Screenshot 2: Footer

new_theme_2

@cintek
Copy link
Author

cintek commented Sep 26, 2023

I have a question: How can I create a util.html that uses the util.html from templates but redefines some macros? I wasn't able to import my own file so I wrote the changed macros right into the layout.html.

@cintek
Copy link
Author

cintek commented Sep 27, 2023

With the last commit I changed the appearance of the modify window for the new theme:

Modify section in new theme

@RogerHaase
Copy link
Member

cintek, please make your moin repo public. It is easier to clone your repo and test all your changes than it is to review and download each diff separately.

@wagner-intevation
Copy link
Contributor

cintek, please make your moin repo public. It is easier to clone your repo and test all your changes than it is to review and download each diff separately.

The repo is at https://github.com/cintek/moin/tree/new-theme

reverse the order of button inputs (sumbit/OK first) in DOM, and thus
submit is the default on enter

to keep the order reversed when viewed (first cancel, then OK), reverse
the display with CSS

fixes moinwiki#1559
@UlrichB22 UlrichB22 changed the title New Theme New theme 'focus' Nov 20, 2023
@cintek cintek marked this pull request as ready for review December 13, 2023 08:23
@cintek
Copy link
Author

cintek commented Dec 13, 2023

I marked this PR as ready for preview since now the most important work is done and it would be great to read what you think about this theme. Does is fulfill your expectations for a theme?

@UlrichB22
Copy link
Collaborator

Thanks for providing the new theme.
I found some minor issues

  • on the +index page the buttons are too small for the labels:

moin_theme_focus_index

  • the name of the theme should be extended to 'Focus Theme'. Then it fits to the other themes in the user settings dialog under Appearance.
  • Can you change the PR to only contain one commit?

@RogerHaase
Copy link
Member

Please add your theme to: http://moinmo.in/ThemeMarket

Also check the moin2 docs to verify there is a pointer to the above theme market, and the moin2 docs have working instructions for installing a contributed theme.

@wagner-intevation
Copy link
Contributor

Please add your theme to: http://moinmo.in/ThemeMarket

Do you mean by that that the theme mustn't be part of the moin repository, but in a separate repository?

@RogerHaase
Copy link
Member

RogerHaase commented Dec 18, 2023 via email

@wagner-intevation
Copy link
Contributor

Thank you for the answer.

Can themes add new translations?

@RogerHaase
Copy link
Member

RogerHaase commented Dec 18, 2023 via email

@wagner-intevation
Copy link
Contributor

@cintek
Do you think it is enough to instead of the current proposal just display a small triangle indicating a small triangle (First is the current version of the theme, the latter is without the new translation)? I don't think it is user-friendly enough and I think we need some good idea to solve that.

image

image

@UlrichB22
Copy link
Collaborator

It should be a rare case that additional translations are required for a new theme. Maybe we can add a dummy function somewhere and include the string 'More'. For javascript there is a similar workaround with src/moin/templates/dictionary.js.

I agree that a triangle without a label is not user-friendly.

@RogerHaase
Copy link
Member

I finally understand issue. Focus theme invents a new word or phrase that needs translation, but contributed themes may not be present when python setup.py extract_messages is run so translation is not possible.

Maybe add a dummy procedure to end of /themes/__init__.py

def contributed_themes():
    """Contributed themes may add translation strings here"""
    Focus_theme = _("More")

@cintek
Copy link
Author

cintek commented Jan 2, 2024

We have at least four options (I add one):

  1. Use only the triangle: I agree that this is not very user-friendly (especially because its such a small icon)
  2. Use only one icon but a different (bigger) one
    • Pro: Better than 1. because easier to see and to hit with the cursor
    • Con: Still no text
  3. Do the translation via JavaScript
    • Pro: Everything belonging to the theme is in one place
    • Con: Needs JavaScript enabled
  4. Use the function @RogerHaase mentioned: What I don't like about this solution is that the code for a contributed theme is distributed to different places - The theme with a separate repository (because it should be offered via the Theme Market) and the function which will part of the moin repository

I tend towards option 3 although it would be better if developers could just add a JSON file or something similar for translations.

@RogerHaase
Copy link
Member

A fifth option is to just use _("More") in the theme code and ignore that a tiny part of the theme is English only for now. If the theme is added to the main repo at some future date, the problem is self-correcting.

I do not understand the Javascript solution - how would that work?

@cintek
Copy link
Author

cintek commented Jan 3, 2024

Here is an example of how it could work. This way you would just need to add the attribute data-i18n to an HTML element and add a translation to the translations.js file. Of course this is a bit over-powered for just one static string so in our case I would write a shorter variant :)

@bernhardreiter
Copy link

Two remarks:

  • We (with @cintek and @wagner-intevation ) aim to have a theme which can replace the standard default theme (because we found it lacking, so in a sense it is not about style but a usability fix to the standard interface).
  • For the translation the function seems the best solution (from what I've read).

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.

None yet

5 participants