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

ui-core: add build instructions to readme file #88

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

turnerian2004
Copy link
Contributor

@turnerian2004 turnerian2004 commented Jul 6, 2024

Issue #89

@turnerian2004
Copy link
Contributor Author

I struggled a lot today to build the project. There may be other commands that are better. Is there something that should be added or removed?

Screenshot 2024-07-06 at 8 46 11 PM

I could not resolve the problems below with this library. What am I doing something wrong? I tried to install the package as specified on the npm page, but I had no luck: https://www.npmjs.com/package/@osrd-project/ui-icons

Screenshot 2024-07-06 at 8 46 59 PM

@turnerian2004
Copy link
Contributor Author

@sim51 @emersion I seem not able to request a review. Would you please review this pr? :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@emersion
Copy link
Member

It seems like this PR touches more than just the README - it also includes some package.json and package-lock.json changes. As said above, I don't think we need these changes.

To sum things up, IMHO npm install + npm run build + npm run storybook should be enough to get people started.

@turnerian2004 turnerian2004 requested a review from a team as a code owner July 11, 2024 18:34
@turnerian2004 turnerian2004 force-pushed the ian/add-readme branch 3 times, most recently from d9b1d95 to ce305ca Compare July 11, 2024 18:42
@Yohh
Copy link
Contributor

Yohh commented Jul 14, 2024

@turnerian2004 that's a good point, basically we run the project with npm run start --ws (after npm install and npm run build) in order to load every workspace in the project, that's not an issue if you need to work for only one specific sub-project, but some of them depends on other ones (osrd-manchette needs osrd-spacetimechart, osrd-speedspacechart needs osrd-core, many of them needs osrd-icons...)

many changes are work-in-progress by @jacomyal , and a different process will be set very soon, but I think it's a good start!

README.md Show resolved Hide resolved
@turnerian2004
Copy link
Contributor Author

I'll get back later this week. I want to do a little reading up, if that's okay.

@turnerian2004
Copy link
Contributor Author

@Yohh @emersion @clarani

Thank you for the feedback on the pr. Much appreciated :) Feel free to make any desired changes to merge the pr :)

README.md Show resolved Hide resolved
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !! ✅

This README really needed this improvement 🙏

@clarani clarani enabled auto-merge July 25, 2024 10:05
@clarani clarani added this pull request to the merge queue Jul 25, 2024
Merged via the queue into OpenRailAssociation:dev with commit eb50698 Jul 25, 2024
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