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

Port to vite #140

Merged
merged 17 commits into from Jun 23, 2022
Merged

Port to vite #140

merged 17 commits into from Jun 23, 2022

Conversation

bluskript
Copy link
Collaborator

@bluskript bluskript commented Jun 14, 2022

Status

Bootswatch needed to be removed due to SASS issues. The UI still looks pretty much the same, but colors are wrong.
This PR might also be used to solve #116 as a result.

Items to test:

  • Production build
  • CI
  • Check that all UI still works correctly
  • update README.md with new dev and release building instructions

@bluskript
Copy link
Collaborator Author

bluskript commented Jun 15, 2022

image

I revamped a large portion of the UI, though a lot of things are broken (since i'm making a lot less stuff use bootstrap and instead use Tailwind). Note that the dark theme was just a thing i copypasted from another project and a light theme can be added super easily when its needed.

There's still some visuals that were lost but i'll bring them back better than they were before soon enough.

Also, as mentioned before, a lot of the UI styling is gone because of SASS issues, which is why it doesn't look good as the main branch, at least for the buttons.

@nathanleclaire
Copy link
Contributor

Fun! I'm a bit confused though, I thought it the migration to vite was just about dev tooling. Maybe split them up into separate PRs?

@bluskript
Copy link
Collaborator Author

bluskript commented Jun 15, 2022

It's kind of a weird one. Vite doesn't like Bootswatch because of its use of url() with relative imports, see vitejs/vite#7651.

This means if this PR merges as is then a lot of the buttons and styling will revert back to the default which doesn't look that great. Since I was planning to tackle #116 anyway, I was going to solve the issue by avoiding Bootstrap for the most part and using Tailwind to style the UI. A lot of the UI needed a styling cleanup anyway.

@nathanleclaire
Copy link
Contributor

Got it. I'm open to it if @SvenDowideit is! I dig the screenshot.

import {
useConfigState,
setConfigValue,
ConfigKey,
Copy link
Member

Choose a reason for hiding this comment

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

I presume these ordering changes are because your local prettifier settings are arguing with ours.

I'd love these settings to be in the project only - so there's no chance that a user's personal choices bleed over.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Argh, yeah this is always a classic frustrating bit with frontend dev - there's so many tools that could interfere with the formatting guidelines. A pre-commit hook would probably be the best approach since it enforces that everything is correct right at the end. I've noticed that there seems to be one but it doesn't run for some reason. I'll look into it.

@SvenDowideit
Copy link
Member

for me, getting rid of erb is a positive - very curious to see how it all hangs together in the end

@bluskript bluskript marked this pull request as ready for review June 22, 2022 05:53
"lint": "cross-env NODE_ENV=development eslint . --ext .js,.jsx,.ts,.tsx",
"package": "ts-node ./.erb/scripts/clean.js dist && npm run build && electron-builder build --publish never",
"postinstall": "ts-node .erb/scripts/check-native-dep.js && electron-builder install-app-deps && cross-env NODE_ENV=development webpack --config ./.erb/configs/webpack.config.renderer.dev.dll.ts",
"start": "ts-node ./.erb/scripts/check-port-in-use.js && npm run start:renderer",
Copy link
Member

Choose a reason for hiding this comment

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

its probably a nice thing to defined npm run start === npm run dev - for the new developer, the latter tells them little about what it does (does it start an editor?)

package.json Outdated
"build:renderer": "vite build --config ./src/renderer/vite.config.ts",
"start:main": "npm run build:main && cross-env NODE_ENV=development electronmon -r ts-node/register/transpile-only ./src/main/main.ts",
"start:renderer": "vite dev --config ./src/renderer/vite.config.ts",
"package": "rimraf ./release && npm run build && electron-builder build --publish never",
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 guessing you've got a bunch of README.md info you want to update?

@@ -73,140 +51,89 @@ declare global {
}
}

function TooltipNavItem({
Copy link
Member

Choose a reason for hiding this comment

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

does this mean there are no defaults anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't exactly find any reason for defaults to be specified. If a nav item was defined, I'd assume that the author of the code would need to fill in all of the properties correctly, right? Why define defaults if they're never (or rather shouldn't) be used?

Copy link
Member

Choose a reason for hiding this comment

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

when i'm prototyping, i appreciate defaults lots.

@SvenDowideit
Copy link
Member

SvenDowideit commented Jun 22, 2022

the size of the diff doesn't worry me, I'm more interested in why choices were made - like replacing higherlevel components with plain div with classes - that always feels like a backward step to me.

very much needs docs to tell us what changed, and how new devs build, run and ship things.

(and presumably we're still waiting on your todo's in this pr)

oh, and the rebase for conflicts :)

@bluskript
Copy link
Collaborator Author

bluskript commented Jun 22, 2022

Much of the layouting pains in the bootstrap version of the app was due to use of Rows incorrectly. Due to Rows containing gutters, it would misalign things because of the negative margin. Tailwind goes for a more simplistic approach by providing atomic classes for your styles. This is more nice than higher level components because:

a) its closer to bare HTML and CSS which may seem like a downside but makes it easier to reason about when it comes to layouting issues
b) more compatible with tailwind's spacing styling. Bootstrap's layouting is very similar to Tailwind's, Tailwind's is just much larger and closer to the original CSS styles.

While bootstrap's Rows and Cols and other high level Bootstrap components appear to be helpful, they also make customizeability much more of a challenge and introduce more room for issues. I've learned its easier honestly to just do the layouting with raw divs, at least as long as Tailwind is there to provide easy atomic classes for it. Not to mention, it's yet another react component in the virtual dom which has its own overhead. Doesn't actually matter, but worth putting out there.

@nathanleclaire
Copy link
Contributor

npm run package seems to be still working 👍

README.md Outdated
$ npm install --legacy-peer-deps && \
(cd ./release/app/ && npm install --legacy-peer-deps) && \
npm install --legacy-peer-deps
$ npm install --legacy-peer-deps
Copy link
Member

Choose a reason for hiding this comment

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

do we need the --legacy-peer-deps anymore? (npm i worked for me on my not-clean box)

package.json Outdated Show resolved Hide resolved
Copy link
Member

@SvenDowideit SvenDowideit left a comment

Choose a reason for hiding this comment

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

YUP, very LGTM

excellent progress

@SvenDowideit SvenDowideit merged commit 429f9f0 into workbenchapp:main Jun 23, 2022
@SvenDowideit SvenDowideit added this to the 0.4.0 milestone Jun 23, 2022
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.

None yet

3 participants