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

Initial PR #1

Merged
merged 9 commits into from
Apr 4, 2022
Merged

Initial PR #1

merged 9 commits into from
Apr 4, 2022

Conversation

derek-heycar
Copy link
Contributor

Initial PR

  • TypeScript
  • eslint, prettier
  • CSS - Stylelint
  • Storybook
  • Lerna
  • IDE .vscode setting
  • Build tool rollup
  • Jest component testing
  • Jest screenshot testing config
  • Initial packages: CSS-vars, Theming, Button

mwagdi
mwagdi previously approved these changes Apr 4, 2022
Copy link

@mwagdi mwagdi left a comment

Choose a reason for hiding this comment

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

@derek-heycar I get Failed to resolve a react-scripts package while attempting to start storybook

@mwagdi mwagdi self-requested a review April 4, 2022 08:28
@mwagdi mwagdi dismissed their stale review April 4, 2022 08:29

Trouble starting storybook

Copy link
Contributor

@hey-igor hey-igor left a comment

Choose a reason for hiding this comment

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

lgtm :) just have one question

@@ -0,0 +1,56 @@
:root {
--font-family-system: Objektiv, sans-serif, 'Helvetica Neue', Helvetica, Arial;
--font-family-styrene: Objektiv, sans-serif, 'Helvetica Neue', Helvetica,
Copy link
Contributor

Choose a reason for hiding this comment

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

what does styrene mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing that. But actually, the css-variables package has not been finished yet. I thought to use it as an intermediate variable for the font, but I've checked, and it's not needed now.
I'll replace it.

test: cssRegex,
}),

// new OptimizeCSSAssetsPlugin({
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rather remove those lines?

@derek-heycar derek-heycar merged commit c810c3d into develop Apr 4, 2022
@derek-heycar derek-heycar deleted the feature/base branch April 4, 2022 12:05
derek-heycar added a commit that referenced this pull request Apr 20, 2022
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.

4 participants