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

Configuration Changes #1

Merged
merged 13 commits into from
Jul 25, 2022
Merged

Configuration Changes #1

merged 13 commits into from
Jul 25, 2022

Conversation

RyanMulready
Copy link
Contributor

@RyanMulready RyanMulready commented Jul 22, 2022

Merge First

Copy link
Member

@tomleo tomleo left a comment

Choose a reason for hiding this comment

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

A little confused about the interplay of webpack/eslint/rollup in es-vue-base but overall changes look good 👍

install:
npm install
npx lerna bootstrap
npm --prefix es-vue-base run build
Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks for working this bit out

@@ -16,4 +20,5 @@ publish:

.PHONY: update
update:
lerna bootstrap
npm --prefix es-vue-base run build
Copy link
Member

Choose a reason for hiding this comment

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

Running make build will trigger the build script in es-vue-base. What do you think about the pattern of make build && make update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I think there's still a few things to figure out here. I'm not convinced this is right yet, I think we should try starting from scratch once this is stable again.

@@ -0,0 +1,151 @@
# EnergySage Design System
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused why this is showing up as an addition on the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just renamed the case readme.md -> README.md

[*]
charset = utf-8
end_of_line = lf
indent_size = 2
Copy link
Member

Choose a reason for hiding this comment

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

Would you be cool with increasing the indent size to 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm cool with it just didn't want to introduce formatting/linting changes in this PR. We should talk more broadly about what to do with es-bs-base and es-bs-extends in terms of linting.

@@ -1,4 +1,3 @@
// EnergySage's Flavor of Bootstrap
@import './variables';
Copy link
Member

Choose a reason for hiding this comment

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

👏

@@ -0,0 +1,73 @@
module.exports = {
settings: {
'import/resolver': {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused how this works in a rollup context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't. Each tool has their own alias resolver. This just tells eslint what @ means in an import statement.

"eslint": "^8.20.0",
"eslint-config-energysage": "^2.2.1",
"eslint-config-prettier": "^8.5.0",
"eslint-import-resolver-webpack": "^0.13.2",
Copy link
Member

Choose a reason for hiding this comment

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

Should https://www.npmjs.com/package/@rollup/plugin-eslint be used in this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't really understand this plugins use case. It seems to just look at the rollup config for files to lint, given we are linting everything I didn't think it was necessary.

@@ -1,77 +1,80 @@
<script>
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this is just prettier updates. FWIW I forgot to delete this file. Hopefully there wasn't any manual work updating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's gone in another PR.

@@ -1,7 +1,12 @@
{
"name": "root",
"private": true,
"scripts": {
"watch": "nodemon --exec make update"
Copy link
Member

Choose a reason for hiding this comment

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

This feels hacky, but I like it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super hacky, not sure we actually want to keep it but it does work. I think you're right that nx will solve this problem for us but I tried for a bit and couldn't get it working.


```bash
$ git remote -v
origin [email protected]:EnergySage/es-ds.git (fetch)
origin [email protected]:EnergySage/es-ds.git (push)
```

Next you should run `make update`. This will kick-off installing requirements for the repo.
Next you should run `make install`. This will kick-off installing requirements for the repo.
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch 👍 , sorry for the confusion here.

@RyanMulready RyanMulready marked this pull request as ready for review July 25, 2022 14:30
@RyanMulready RyanMulready merged commit 4e368aa into main Jul 25, 2022
@RyanMulready RyanMulready deleted the es-ds-config-updates branch July 25, 2022 14:30
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