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

Add base styles file and refactor styles in boilerplate app #2717

Merged
merged 8 commits into from
Jul 30, 2024

Conversation

fpena
Copy link
Collaborator

@fpena fpena commented Jul 29, 2024

Please verify the following:

  • yarn test jest tests pass with new tests, if relevant
  • yarn lint eslint checks pass with new code, if relevant
  • yarn format:check prettier checks pass with new code, if relevant
  • README.md (or relevant documentation) has been updated with your changes

Describe your PR

This PR:

  • Adds a new file for base styles: boilerplate/app/theme/styles.ts
  • Refactor some common styles in the boilerplate app

@fpena fpena changed the title Feat/add base styles Add base styles file and refactor styles in boilerplate app Jul 30, 2024
@fpena fpena marked this pull request as ready for review July 30, 2024 17:54
Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

Nice work!

One minor fix to make I think, otherwise good to go!

Comment on lines +8 to +11
container: {
paddingTop: spacing.lg + spacing.xl,
paddingHorizontal: spacing.lg,
} as ViewStyle,
Copy link
Contributor

Choose a reason for hiding this comment

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

These look to only appear on the demo app screens. We should wrap a comment block around here for removing this block since it'll be unused if/when the user removes the demo code.

You can test the difference with --remove-demo (or the Y flag when it asks if you want to remove the demo app) and see that it gets removed or remains accordingly.

Suggested change
container: {
paddingTop: spacing.lg + spacing.xl,
paddingHorizontal: spacing.lg,
} as ViewStyle,
// @demo remove-block-start
container: {
paddingTop: spacing.lg + spacing.xl,
paddingHorizontal: spacing.lg,
} as ViewStyle,
// @demo remove-block-end

Copy link
Contributor

Choose a reason for hiding this comment

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

@fpena be sure to also add this file to the remove file: test/vanilla/__snapshots__/ignite-remove-demo.test.ts.snap

Copy link
Contributor

@mazenchami mazenchami left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

🚀

@fpena fpena merged commit d3d8b38 into v10 Jul 30, 2024
1 check passed
@fpena fpena deleted the feat/add-base-styles branch July 30, 2024 20:45
markrickert added a commit that referenced this pull request Jul 30, 2024
@infinitered-circleci
Copy link

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants