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

Compile with parcel #4

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Compile with parcel #4

wants to merge 15 commits into from

Conversation

yakov116
Copy link

@yakov116 yakov116 commented Apr 28, 2021

Basically what I did here is compile the website with parcel.
Using gh-pages you can deploy this directly.

There was no code changes except for some linting (Use ES6 syntax etc.)

I also removed non used dependencies (Jquery and Bootstrap Java script files) this make the fast website load even faster

You can test it out on https://yakov116.github.io/isyontefearlythisyear/

I am not finished linting. I would just like to know if you would accept this.

Also needed would be to enable actions and create the workflow file

@yakov116 yakov116 marked this pull request as draft April 28, 2021 20:16
@yakov116 yakov116 marked this pull request as ready for review April 28, 2021 20:24
@Noleli
Copy link
Owner

Noleli commented Apr 29, 2021

Hey, thanks for this. To be honest, I’ve never used any sort of bundler (or any sort of JS toolchain, really — my projects tend to be pretty small) so I don’t really know what the advantages are here. It seems cool that you can easily deploy it to gh-pages. Before merging this in, I’d like to know how to build/deploy it. I’d install parcel and parcel index.html?

Thanks for doing the linting! Several months ago I was looking at optimizing and was pretty sure it wasn’t using jQ, but couldn’t remember for sure from when I first wrote this in 2018.

@yakov116
Copy link
Author

I’d install parcel and parcel index.html?

Yes.

You can also just do npm run watch

@yakov116
Copy link
Author

I’d like to know how to build/deploy it

You would just need to add the following file to master branch. Enable GitHub pages to a clean branch and 🎉

https://github.com/yakov116/isyontefearlythisyear/blob/master/.github/workflows/demo.yml

@yakov116
Copy link
Author

https://observablehq.com/embed/@d3/d3v6-migration-guide?cells=group

it would be nice to be able to upgrade to d3 v6 but I am not sure what to replace there.

The whole file will be cut in 1/2

@yakov116
Copy link
Author

You would just need to add the following file to master branch. Enable GitHub pages to a clean branch and 🎉

https://github.com/yakov116/isyontefearlythisyear/blob/master/.github/workflows/demo.yml

I added the file to this PR it will enable it after merging

@Noleli
Copy link
Owner

Noleli commented Apr 29, 2021

https://observablehq.com/embed/@d3/d3v6-migration-guide?cells=group

it would be nice to be able to upgrade to d3 v6 but I am not sure what to replace there.

The whole file will be cut in 1/2

Wow, is v6 that much smaller‽ I didn’t realize that. It has a number of breaking changes with v5 because it relies on newer built-in JS data structures like maps, which is great, but means updating the way isyontefearlythisyear aggregates data. I don’t think it’d actually be that hard, but this project has kind of been in launch-and-forget mode for a while.

@yakov116
Copy link
Author

Its modual based which means that when compiling it will only bring the functions we use.

V5 imports everything

@Noleli
Copy link
Owner

Noleli commented May 2, 2021

It seems like a lot of your linting is making style changes that are opinion based. Out of curiosity, are there advantages to putting spaces before function arguments, using single-quote strings, and using tab rather than 4-space indenting? (I agree with you on using const where appropriate and using ${} string interpolation rather than concatenation.)

@yakov116
Copy link
Author

yakov116 commented May 2, 2021

My personal style. I can change the setting in the linter.

spaces before function arguments,

This is the only one that I really like, makes it easier to read. IMHO

@yakov116
Copy link
Author

yakov116 commented Dec 5, 2021

Bump

@yakov116
Copy link
Author

yakov116 commented Dec 9, 2021

@Noleli ?

@Noleli
Copy link
Owner

Noleli commented Dec 10, 2021

Hi Yakov, sorry for the delay, and thanks again for your contribution!

I’m good with most of the linting and getting rid of the unused dependencies, but really don’t think a project of this size is large enough to warrant a build tool. The one linting thing I disagree with is the replacement for Array.forEach() loops with for(i in a) loops (I really like the array methods :) ). If you remove Parcel and change that one linter setting I’d be happy to merge in.

@yakov116
Copy link
Author

The whole point of the PR was to use Parcel so it can be deployed via GitHub pages (making load time instant since everything is compiled and no need to download any files.)

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