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

feat(webapp): modernize development tooling - replace webpack, craco, with vite #413

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

benpsnyder
Copy link
Contributor

Least amount of work possible to get webapp running with Vite.

Did minimal changes from #399

Copy link

vercel bot commented Apr 21, 2024

@benpsnyder is attempting to deploy a commit to the Bigcapital Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@abouolia abouolia left a comment

Choose a reason for hiding this comment

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

To reduce the new additions on this PR, I suggest it would better if we split the replacing class to className part to separate PR. I would do tomorrow if you're busy. @benpsnyder

@@ -7,25 +6,27 @@ import { SubscriptionPlansSection } from './SubscriptionPlansSection';
import withSubscriptionPlansActions from '../../Subscriptions/withSubscriptionPlansActions';
import styles from './SetupSubscription.module.scss';

// TODO: use typescript https://github.com/NuroDev/lemonsqueezy.ts/tree/main/src/modules/subscription#-subscription
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abouolia kindly make the lemonsqueezy implementation type-safe. I had to disable this to get Vite to run. I can't test this functionality so I don't want to try to restore the functionality

@benpsnyder
Copy link
Contributor Author

To reduce the new additions on this PR, I suggest it would better if we split the replacing class to className part to separate PR. I would do tomorrow if you're busy. @benpsnyder

Yes, the bulk of the file changes is a simple change from class= to className= but without the change, the UI doesn't render right.

if you leave it as class=
image

changed to className= which is proper ...
image

@benpsnyder benpsnyder force-pushed the feat/webpack-to-vite branch from 1024f74 to d8c1df5 Compare April 22, 2024 13:43
@abouolia
Copy link
Contributor

abouolia commented Apr 29, 2024

@benpsnyder I have looked at the PR, adding tailwindcss is something we avoided to do, for several reasons but the main reason because Blueprintjs is our primary library components and BP does not support tailwindcss styling, we wanna follow the same styling approach of Blueprintjs to add consistency in our project. in the beginning we followed the styled components but now the CSS Module is used.

but the PR has a lot can benefit of it, for instance replacing "class=" to "className" becuase as you know className is more for React and we'd like to replace it on the whole project.

@benpsnyder
Copy link
Contributor Author

@abouolia I will remove the tailwindcss part of this PR so you can complete it. I disagree with not putting tailwindcss into the project but we can take up a discussion about that another time. At the end of the day I think it is valuable to keep momentum of closing PRs that add incremental value to the project.

@benpsnyder benpsnyder changed the title feat(webapp): modernize development tooling - replace webpack, craco, with vite and add support for tailwindcss feat(webapp): modernize development tooling - replace webpack, craco, with vite Apr 29, 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.

2 participants