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

DEV: Migrate icons and links settings to new objects setting type #36

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

tgxworld
Copy link
Contributor

@tgxworld tgxworld commented Apr 5, 2024

Why this change?

Since discourse/discourse@a440e15, we have started to support objects typed theme setting so we are switching this theme component to use it instead as it provides a much better UX for configuring the settings required for the theme component.

@tgxworld tgxworld force-pushed the migrate_settings branch 3 times, most recently from 596bd24 to d3362ce Compare April 5, 2024 03:53
@@ -1,3 +1,4 @@
< 3.3.0.beta2-dev: 0a837d7c044798417b1a7389cc37a67859c25dbe
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get this SHA from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch. I have no idea where that SHA came from but I have now updated the SHA to point to e64cfa5

Since discourse/discourse@a440e15, we have started to support objects typed theme setting so we are switching this theme component to use it instead as it provides a much better UX for configuring the settings required for the theme component.
@@ -1,3 +1,4 @@
< 3.3.0.beta2-dev: e64cfa531121f2cf152901f5b2a20512a5ee2c63
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type: enum
default: _blank
choices:
- _blank
Copy link
Contributor

@nattsw nattsw Apr 24, 2024

Choose a reason for hiding this comment

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

Btw, why the _? I notice it wasn't a thing in discourse/discourse-custom-header-links#53. Need some standardization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In discourse-custom-header-links it doesn't just use the target directly in the link's target attribute.

https://github.com/discourse/discourse-custom-header-links/blob/6bd25fa7d740bd4888ddd8af3802feecc6e19bc2/javascripts/discourse/components/custom-header-links.js#L28

In the case of this plugin, the value of the property is used directly in the target attribute so we need to use the actual valid values

@nattsw
Copy link
Contributor

nattsw commented Apr 24, 2024

Screenshot 2024-04-24 at 11 28 31 AM

You cancelled the test run?

@tgxworld
Copy link
Contributor Author

Hmm nope, not sure what happened. I kicked off another rerun.

@tgxworld tgxworld merged commit 7e304e0 into main Apr 24, 2024
4 checks passed
@tgxworld tgxworld deleted the migrate_settings branch April 24, 2024 05:20
@tgxworld
Copy link
Contributor Author

Thank you for reviewing @nattsw 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants