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

Migration to @dooboo-ui/theme #150

Merged
merged 5 commits into from
Oct 16, 2021
Merged

Migration to @dooboo-ui/theme #150

merged 5 commits into from
Oct 16, 2021

Conversation

yujonglee
Copy link
Contributor

@yujonglee yujonglee commented Oct 15, 2021

Description

This is a follow-up to the previous work #149 .

TL;DR :
@dooboo-ui/theme works fine independently. (Link)
But with something related to build / commonjs esmodule / JSX / transpiling, it fails to work in dooboo-ui.


Currently, there's some problem that need to be fixed. Here's current status.
1. Demo -> Web(X - build fails) iOS(O : totally fine) Android(Not tried but might same as iOS)
스크린샷 2021-10-14 오후 10 24 31

2. Docs -> Only Web (X - build complete, but component page not shown)
Getting Started, Theming, Contributing works, but rest doesn't work.

스크린샷 2021-10-15 오후 9 14 30

3. Test -> Fails. Can't read @dooboo-ui/theme.

Current:

When I build @dooboo-ui/theme with "compilerOptions": { "module": "commonjs" }, :

Test Plan

None.

Related Issues

#104, #105, #129

Tests

None.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [] Run yarn test:all and make sure nothing fails. You can run yarn test -u to update snapshots if needed.
  • I am willing to follow-up on review comments in a timely manner.

@yujonglee yujonglee added 🙏 help wanted Extra attention is needed 🚽 migration Activities due to changes in framework labels Oct 15, 2021
@github-actions
Copy link

github-actions bot commented Oct 15, 2021

Visit the preview URL for this PR (updated for commit f830be4):

https://dooboo-ui--pr150-theme-migrate-73yrqypm.web.app

(expires Sat, 30 Oct 2021 15:49:12 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@hyochan
Copy link
Member

hyochan commented Oct 16, 2021

@yujong-lee I've made some fixes in e9ca7c8 and 8c82d93 and fix the problem you've stated.

Please fix the remaining tests failing in ButtonGroup.test.tsx which you've worked on lastly.

@hyochan hyochan marked this pull request as ready for review October 16, 2021 15:33
@hyochan hyochan removed a link to an issue Oct 16, 2021
@hyochan hyochan linked an issue Oct 16, 2021 that may be closed by this pull request
@hyochan hyochan removed a link to an issue Oct 16, 2021
@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Merging #150 (f830be4) into master (5bd77a4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #150   +/-   ##
=======================================
  Coverage   94.86%   94.86%           
=======================================
  Files          28       28           
  Lines         682      682           
  Branches      306      306           
=======================================
  Hits          647      647           
  Misses         35       35           

Copy link
Member

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

Great work 💯

@hyochan hyochan merged commit 612f661 into master Oct 16, 2021
hyochan added a commit that referenced this pull request Oct 16, 2021
@yujonglee yujonglee deleted the theme/migrate branch October 17, 2021 10:34
@hyochan hyochan mentioned this pull request Oct 18, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙏 help wanted Extra attention is needed 🚽 migration Activities due to changes in framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detach main theme from dooboo-ui and maintain seperately
2 participants