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

Fix import stories/package/CalendarCarousel directory #49

Closed
wants to merge 1 commit into from
Closed

Fix import stories/package/CalendarCarousel directory #49

wants to merge 1 commit into from

Conversation

fronttigger
Copy link
Contributor

@fronttigger fronttigger commented Aug 7, 2021

Description

Error occurs at yarn start after initial git clone.
There is no lib in the packages/CalendarCarousel directory.

Related Issues

X

Tests

X

Checklist

Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Run yarn test or yarn test -u if you need to update snapshot.
  • Run yarn lint
  • I am willing to follow-up on review comments in a timely manner.

@hyochan hyochan added the 🛠 bugfix All kinds of bug fixes label Aug 7, 2021
@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #49 (a9ac7a5) into master (42dafcd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   94.09%   94.09%           
=======================================
  Files          21       21           
  Lines         542      542           
  Branches      231      231           
=======================================
  Hits          510      510           
  Misses         32       32           

@fronttigger fronttigger changed the title change import stories/package/CalendarCarousel directory fix change import stories/package/CalendarCarousel directory Aug 7, 2021
@fronttigger fronttigger changed the title fix change import stories/package/CalendarCarousel directory Fix import stories/package/CalendarCarousel directory Aug 7, 2021
@hyochan
Copy link
Member

hyochan commented Aug 7, 2021

I might have to tell you a short story on the design for dooboo-ui. I am sorry that we did not make this clear enough in our doc 🙏.
According to unifying dooboo-ui-legacy roadmap, our initial strategy to bring our project further was dividing into 3 layers.

  1. The main components
  2. The separate package components - Those include other third-party modules
  3. Theme package

However, now we have only 1 and 2 since the theme is merged with the main components.

For CalendarCarousel, since we use an extra package intl, we moved this component to packages instead of main. The packages modules are maintained with lerna to maintain the mono repo. Each package component is published in a separate package like @dooboo-ui/calendar-carousel.

Technically, we are managing each package component with package.json and since we are using typescript, each of them needs to be built and package.json should point to the main script like below.
Screen Shot 2021-08-07 at 11 52 46 PM

Therefore, we currently have a bit inconvenient way to develop the package component. I'd suggest you run yarn pre to correctly configure the package components and it is better to be provided in postinstall step rather than changing the import path since that is not how packages are maintained.

Further Tips

When you are developing a package component, we recommend you to watch typescript using -w flag or temporarily rename package.json in the package component and revert it before committing.

Please tell me if anything else is not clear here. I also want to find out a better way to maintain package components. Looking foward to seeing if there is any alternative solution :)

@hyochan hyochan added 💬 discussion Discuss issues 🦢 maintenance and removed 🛠 bugfix All kinds of bug fixes labels Aug 7, 2021
@fronttigger
Copy link
Contributor Author

@hyochan
Thank you for the explanation. I'm not used to it yet, so I didn't know.
I'll refer to this part and try again. 👍

@hyochan
Copy link
Member

hyochan commented Aug 7, 2021

@hyochan
Thank you for the explanation. I'm not used to it yet, so I didn't know.
I'll refer to this part and try again. 👍

No worry! I could provide information because of your commitment. This is usually how opensource improves (we need thread and avoid no thread 🤤).

Furthermore, understanding the comment and updating the doc would be very valuable! It is ok if you continue your work on current PR!

@fronttigger fronttigger closed this Aug 7, 2021
@fronttigger fronttigger deleted the storybook/calendarCarousel branch August 7, 2021 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants