Skip to content

theme [nfc]: Make DesignVariables.{light,dark} final fields, not constructors #1236

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

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

chrisbobbe
Copy link
Collaborator

Nicer to compute each of these just once.

@gnprice
Copy link
Member

gnprice commented Dec 30, 2024

(nit: the status quo is they're constructors, not methods)

@chrisbobbe chrisbobbe changed the title theme [nfc]: Make DesignVariables.{light,dark} final fields, not methods theme [nfc]: Make DesignVariables.{light,dark} final fields, not constructors Dec 30, 2024
@chrisbobbe
Copy link
Collaborator Author

Indeed :) thanks for the catch

@chrisbobbe chrisbobbe force-pushed the pr-design-variables-final-fields branch from aeb0f73 to 8cb19bb Compare December 30, 2024 20:06
@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Dec 30, 2024
@PIG208 PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jan 8, 2025
@PIG208
Copy link
Member

PIG208 commented Jan 8, 2025

LGTM! We should be able to apply this to MessageListTheme and etc., right?

@PIG208 PIG208 requested review from gnprice and removed request for PIG208 January 8, 2025 04:52
Instead of constructors. Nicer to compute each of these just once.
@gnprice gnprice force-pushed the pr-design-variables-final-fields branch from 8cb19bb to dcc8123 Compare January 10, 2025 07:31
@gnprice gnprice merged commit dcc8123 into zulip:main Jan 10, 2025
1 check passed
@gnprice
Copy link
Member

gnprice commented Jan 10, 2025

Thanks! Looks good; merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants