Show topic counts for custom nav links matching /new and /unread and Add per-language translation support for custom nav links - #58
Conversation
Custom nav links pointing to /new or /unread now display the topic count in parentheses (e.g., "New (24)"), matching the behavior of Discourse's built-in navigation items. The count is retrieved from the TopicTrackingState service and updates reactively. Links with other URLs remain unchanged.
697d81a to
22a1303
Compare
akanora
changed the title
|
This looks good. Do you mind adding some tests? |
pmusaraj
left a comment
pmusaraj
left a comment
There was a problem hiding this comment.
Also, linting should be addressed.
| url: | ||
| label: URL | ||
| description: The link (e.g., /latest or https://discourse.org) | ||
| translations: |
There was a problem hiding this comment.
Did you look into https://meta.discourse.org/t/add-localizable-strings-to-themes-and-theme-components/109867 for the translations? Is it possible to expose the field values so that they can be translated using the theme translations provided by default?
There was a problem hiding this comment.
I looked into the theme translations approach (locales/*.yml + i18n(themePrefix(...))). Unfortunately, I think it's not feasible for this use case. Theme translations require pre-defined keys at development time, but nav link display names and titles are dynamic values configured by the admin through type: objects settings. When an admin adds a new link (e.g. "FAQ"), there's no corresponding translation key in the locale files, and admins can't create new keys through the Theme Translations UI they can only edit existing ones.
Instead, I've added a nested translations objects property inside each nav link, where admins can add per-locale overrides with a locale dropdown (type: enum with all Discourse-supported locales) and translated display_name/title fields. At runtime, I18n.currentLocale() is matched against these entries (exact match first, then prefix fallback), falling back to the base values for any unconfigured language.
| "/unread": "unread", | ||
| }; | ||
|
|
||
| function getCountForFilter(topicTrackingState, filterType) { |
There was a problem hiding this comment.
Topic counts look reasonable. Would be good to add a test for these as @Grubba27 noted.
There was a problem hiding this comment.
Could you clarify what kind of tests you'd like?
There was a problem hiding this comment.
A qunit or system test that uses the theme component, and validates that the New / Unread count is shown in the item.
5bfb680 to
7898653
Compare
Grubba27
left a comment
Grubba27
left a comment
There was a problem hiding this comment.
Looks good I would just remove the console log from the test file(line 62)
|
tests failed @akanora. Maybe something is missing in the setup of the test |
will try few things |
8ff6f33 to
c310f81
Compare
| let originalNavLinks; | ||
| let originalShowCounts; |
There was a problem hiding this comment.
Is it really necessary to have these variables? If you remove them, would it work?
There was a problem hiding this comment.
They're there to reset settings back to its original state after the test runs, since settings is a shared object that isn't automatically reset between tests. Without the afterEach restore, my mutations to nav_links and Show_counts would leak into other tests that run later, potentially causing unrelated failures that are hard to trace back. The test itself would still pass without them, but it's insurance against test pollution.
There was a problem hiding this comment.
But the honest answer is: it depends on how settings is initialized/reset in this specific test environment, and I haven't confirmed that.
There was a problem hiding this comment.
is there anything else I can do?
There was a problem hiding this comment.
I think all is good, you may merge it!
There was a problem hiding this comment.
I can do the merging? Do I even have permission for that?
There was a problem hiding this comment.
hmm, I think not. It is fine, I'll merge it. Thanks a lot for the contribution!
3 participants
Custom nav links pointing to /new or /unread now display the topic count in parentheses (e.g., "New (24)"), matching the behavior of Discourse's built-in navigation items. The count is retrieved from the TopicTrackingState service and updates reactively. Links with other URLs remain unchanged.
Admins can now provide locale-specific overrides for display_name and title on each nav link, while locales without an override automatically fall back to the default value.