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

FEATURE: Back button for schema theme settings #25743

Merged
merged 2 commits into from Feb 20, 2024

Conversation

OsamaSayegh
Copy link
Member

@OsamaSayegh OsamaSayegh commented Feb 19, 2024

Continue from #25673.

This PR adds a back button to the navigation tree of the schema.

@github-actions github-actions bot added the i18n PRs which update English locale files or i18n related code label Feb 19, 2024
@OsamaSayegh OsamaSayegh force-pushed the feature/back-button-schema-theme-setting branch 2 times, most recently from 19df465 to 1a09ec5 Compare February 19, 2024 17:45
@OsamaSayegh OsamaSayegh force-pushed the feature/back-button-schema-theme-setting branch from 1a09ec5 to 4a7684c Compare February 19, 2024 17:46
@OsamaSayegh OsamaSayegh marked this pull request as ready for review February 19, 2024 18:12
@@ -25,17 +27,16 @@ class Tree {
@tagName("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this while here? 🙂 (it's a no-op, that only applies to non-glimmer components)

@@ -25,17 +27,16 @@ class Tree {
@tagName("")
export default class AdminThemeSettingSchema extends Component {
@tracked activeIndex = 0;
@tracked backButtonText = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to initialize null fields

Suggested change
@tracked backButtonText = null;
@tracked backButtonText;

assert.equal(tree.length, 2);
assert.true(tree[0].active, "the first node is active");
assert.false(tree[1].active, "other nodes are not active");
assert.equal(tree.nodes.length, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't lint against it yet, but assert.strictEqual() is preferred over assert.equal()

Copy link
Contributor

@tgxworld tgxworld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me and is simple to follow 👍 Great work!

There are some minor comments which I think should be addressed before merging though.

@OsamaSayegh OsamaSayegh merged commit 866193e into main Feb 20, 2024
19 checks passed
@OsamaSayegh OsamaSayegh deleted the feature/back-button-schema-theme-setting branch February 20, 2024 10:43
renato pushed a commit to renato/discourse that referenced this pull request Feb 29, 2024
Continue from discourse#25673.

This commit adds a back button to the navigation tree of the schema.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n PRs which update English locale files or i18n related code
3 participants