-
Notifications
You must be signed in to change notification settings - Fork 16
Show topic counts for custom nav links matching /new and /unread and Add per-language translation support for custom nav links #58
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
Changes from all commits
22a1303
2812d3d
3c68250
7f9783b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { acceptance, exists } from "discourse/tests/helpers/qunit-helpers"; | ||
| import { visit } from "@ember/test-helpers"; | ||
| import { test } from "qunit"; | ||
|
|
||
| acceptance("Custom Top Navigation Links | Topic Counts", function (needs) { | ||
| needs.user(); | ||
|
|
||
| let originalNavLinks; | ||
| let originalShowCounts; | ||
|
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it really necessary to have these variables? If you remove them, would it work?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is fine. All good
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there anything else I can do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think all is good, you may merge it!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can do the merging? Do I even have permission for that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, I think not. It is fine, I'll merge it. Thanks a lot for the contribution! |
||
|
|
||
| needs.hooks.beforeEach(function () { | ||
| originalNavLinks = settings.nav_links; | ||
| originalShowCounts = settings.Show_counts; | ||
|
|
||
| settings.nav_links = [ | ||
| { display_name: "New Topics", title: "new topics", url: "/new" }, | ||
| { display_name: "Unread Topics", title: "unread topics", url: "/unread" }, | ||
| ]; | ||
| settings.Show_counts = true; | ||
| }); | ||
|
|
||
| needs.hooks.afterEach(function () { | ||
| settings.nav_links = originalNavLinks; | ||
| settings.Show_counts = originalShowCounts; | ||
| }); | ||
|
|
||
| test("shows new and unread counts in navigation items", async function (assert) { | ||
| const topicTrackingState = this.owner.lookup( | ||
| "service:topic-tracking-state" | ||
| ); | ||
| topicTrackingState.countNew = () => 42; | ||
| topicTrackingState.countUnread = () => 17; | ||
|
|
||
| await visit("/latest"); | ||
|
|
||
| assert.ok( | ||
| exists(".nav-item_custom_new-topics"), | ||
| "custom navigation item for new topics is rendered" | ||
| ); | ||
| assert | ||
| .dom(".nav-item_custom_new-topics a") | ||
| .hasText("New Topics (42)", "it shows the custom new topics count"); | ||
|
|
||
| assert.ok( | ||
| exists(".nav-item_custom_unread-topics"), | ||
| "custom navigation item for unread topics is rendered" | ||
| ); | ||
| assert | ||
| .dom(".nav-item_custom_unread-topics a") | ||
| .hasText("Unread Topics (17)", "it shows the custom unread topics count"); | ||
| }); | ||
| }); | ||
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.
Topic counts look reasonable. Would be good to add a test for these as @Grubba27 noted.
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.
Could you clarify what kind of tests you'd like?
Uh oh!
There was an error while loading. Please reload this page.
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.
A qunit or system test that uses the theme component, and validates that the New / Unread count is shown in the item.