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

[Theming] Add TYPOGRAPHY_CONSTANTS #520

Merged
merged 4 commits into from
Nov 11, 2020

Conversation

michaeljaltamirano
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano commented Nov 11, 2020

What & Why

  • Adds TYPOGRAPHY_CONSTANTS support to theming (with some naming changes).
  • Updates our compareObjectsKeysLength helper to include support for nested objects, along with error messages for why the test failed:
    - Objects of different lengths:Screen Shot 2020-11-11 at 12 04 20 PM
    - Objects of same length with non-matching keys: Screen Shot 2020-11-11 at 12 23 34 PM
  • Adds test for primaryTheme and secondaryTheme having the same keys
    • Probably overkill, but why not.


export const primaryTheme = {
__type: 'primary',
BOX_SHADOW: {},
COLORS: PRIMARY_COLORS,
FONTS: PRIMARY_FONTS,
TYPOGRAPHY: PRIMARY_TYPOGRAPHY,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with just TYPOGRAPHY instead of TYPOGRAPHY_CONSTANTS since we're accessing it off the theme now, so it is more obvious they are constants: there's no need to distinguish between TYPOGRAPHY and TYPOGRAPHY_CONSTANTS since they're not being imported from the same place like we currently do.

@@ -1,9 +1,11 @@
import PRIMARY_COLORS from '../colors/primary';
import PRIMARY_FONTS from '../fonts/primary';
import { PRIMARY_TYPOGRAPHY } from '../typography/primaryTypography';
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 general I think named exports are preferable

@@ -0,0 +1,19 @@
// TODO: Update when values are finalized
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the values the same for now, rather than do empty strings like we are for Colors.

@@ -0,0 +1,18 @@
const fontSize = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These values are duplicated from src/constants/typography/index because I don't think we should be wrapping them in throwOnUndefinedProperty. The value-add is less certain the more we have converted over to TS.

Copy link
Contributor

@daigof daigof left a comment

Choose a reason for hiding this comment

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

👍

@michaeljaltamirano michaeljaltamirano merged commit 8934bd4 into master Nov 11, 2020
@michaeljaltamirano michaeljaltamirano deleted the theming/typography-constants branch November 11, 2020 18:45
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.

3 participants