Skip to content

Change the default theme to AceAttorney#1030

Merged
stonedDiscord merged 2 commits into
masterfrom
defaulttheme
Aug 2, 2024
Merged

Change the default theme to AceAttorney#1030
stonedDiscord merged 2 commits into
masterfrom
defaulttheme

Conversation

@stonedDiscord
Copy link
Copy Markdown
Member

No description provided.

@Salanto
Copy link
Copy Markdown
Contributor

Salanto commented Jul 30, 2024

Are you sure we should change it in client instead of just swapping the theme files in the theme repo?

@stonedDiscord
Copy link
Copy Markdown
Member Author

stonedDiscord commented Jul 31, 2024

I've considered it but it will mess with users who upgrade.
I'd prefer if the theme doesn't suddenly change for existing users who are on the default theme and like it.
This change will basically only affect users who don't have a config.ini yet (or deleted it)

@Salanto
Copy link
Copy Markdown
Contributor

Salanto commented Jul 31, 2024

I just looked over the functions using default_theme are pathing related, so this might break if AceAttorney does not include all elements the default theme provides (like for effects). Otherwise yeah, it would be fine.

@stonedDiscord
Copy link
Copy Markdown
Member Author

Yeah, I'll move the files over later today.

Copy link
Copy Markdown
Contributor

@Salanto Salanto left a comment

Choose a reason for hiding this comment

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

No objections to a merge when the theme repo is updated.

@stonedDiscord
Copy link
Copy Markdown
Member Author

@Salanto
Copy link
Copy Markdown
Contributor

Salanto commented Aug 1, 2024

Just tested this, it will jack up the default theme either way since default contains few items, which will then be loaded from AceAttorney.

@Salanto
Copy link
Copy Markdown
Contributor

Salanto commented Aug 1, 2024

LGTM

@stonedDiscord stonedDiscord merged commit f0c594a into master Aug 2, 2024
@stonedDiscord stonedDiscord deleted the defaulttheme branch August 2, 2024 18:43
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.

2 participants