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

Content refactor #66

Merged
merged 60 commits into from
May 25, 2020
Merged

Content refactor #66

merged 60 commits into from
May 25, 2020

Conversation

gidsi
Copy link
Member

@gidsi gidsi commented Nov 30, 2019

I think we're far enough with the refactoring that the new content is workable and better then what we currently have.
These is still a lot of room to improve, but i think thats easier with having the site online and get used.

@gidsi gidsi requested review from dbrgn and rnestler November 30, 2019 01:56
@gidsi
Copy link
Member Author

gidsi commented Nov 30, 2019

Also fixes #54

@dbrgn
Copy link
Contributor

dbrgn commented Dec 7, 2019

Hmm, I can't get the styling to generate properly... Even after deleting the whole repo, cloning it again and regenerating the website (lektor server -f webpack) it looks like this:

website

I also had to use nvm to ensure that node <=12 is being used. Node 13 (which is installed on my system) breaks node-sass apparently.

PS: Why again do we need Webpack? Is it only for SCSS compilation? Isn't there a solution for Lektor that doesn't pull in a whole jungle of NPM dependencies including – right now – "63 high severity vulnerabilities"? (Yes, I'm aware that these are only build-time dependencies, but still, I think we could probably do without them with just a few tweaks to our JS files. It's not hard anymore nowadays to write JS that runs in most browsers without using Babel. And we don't need to support IE/Edge for our target user group, so assuming a modern browser is fine.)

@gidsi
Copy link
Member Author

gidsi commented Dec 7, 2019

Hmm, I can't get the styling to generate properly... Even after deleting the whole repo, cloning it again and regenerating the website (lektor server -f webpack) it looks like this:

website

I also had to use nvm to ensure that node <=12 is being used. Node 13 (which is installed on my system) breaks node-sass apparently.

PS: Why again do we need Webpack? Is it only for SCSS compilation? Isn't there a solution for Lektor that doesn't pull in a whole jungle of NPM dependencies including – right now – "63 high severity vulnerabilities"? (Yes, I'm aware that these are only build-time dependencies, but still, I think we could probably do without them with just a few tweaks to our JS files. It's not hard anymore nowadays to write JS that runs in most browsers without using Babel. And we don't need to support IE/Edge for our target user group, so assuming a modern browser is fine.)

I've already removed SCSS from webpack in that branch, i forgot to fix the README /package json so it broke on your system :)

@gidsi
Copy link
Member Author

gidsi commented Dec 7, 2019

PS: Why again do we need Webpack? Is it only for SCSS compilation? Isn't there a solution for Lektor that doesn't pull in a whole jungle of NPM dependencies including – right now – "63 high severity vulnerabilities"? (Yes, I'm aware that these are only build-time dependencies, but still, I think we could probably do without them with just a few tweaks to our JS files. It's not hard anymore nowadays to write JS that runs in most browsers without using Babel. And we don't need to support IE/Edge for our target user group, so assuming a modern browser is fine.)

In general i would like to keep Webpack, i know it feels icky sometimes but for me that's an okay-ish trade off to be able to write ES 6/Type Script instead of ES5 (constants, let variables, arrow functions, default values, promises etc etc, here is a complete overview http://es6-features.org).

I would be fine removing Webpack again if where removing the self written js from it too, maybe give the validator a GUI or something instead that we can link.

@dbrgn
Copy link
Contributor

dbrgn commented Dec 7, 2019

constants, let variables, arrow functions, default values, promises etc etc, here is a complete overview http://es6-features.org

You can use all those features right now and they'll work on any browser that's not terribly outdated. Main thing that doesn't work without webpack is ES modules, but right now we don't use those anyways (and most projects can still be imported using script tags).

Maybe we could extract the validator GUI to a separate JS project and make that embeddable (e.g. by providing a DOM element reference to the constructor where the validator GUI can insert itself). For those ~5 lines of JS code we wouldn't need Webpack anymore. I think it would be nice if it's embedded on the website, and not a separate project.

@dbrgn
Copy link
Contributor

dbrgn commented Dec 7, 2019

I've already removed SCSS from webpack in that branch, i forgot to fix the README /package json so it broke on your system :)

Maybe we should include a Makefile with a "run" target 🙂

@gidsi
Copy link
Member Author

gidsi commented Dec 7, 2019

constants, let variables, arrow functions, default values, promises etc etc, here is a complete overview http://es6-features.org

You can use all those features right now and they'll work on any browser that's not terribly outdated. Main thing that doesn't work without webpack is ES modules, but right now we don't use those anyways (and most projects can still be imported using script tags).

Oh... we're already at 96% support in all browsers, you're right, might have been some time when i last checked. Then of course we can rip it out.

Maybe we could extract the validator GUI to a separate JS project and make that embeddable (e.g. by providing a DOM element reference to the constructor where the validator GUI can insert itself). For those ~5 lines of JS code we wouldn't need Webpack anymore. I think it would be nice if it's embedded on the website, and not a separate project.

Yeah, something like this, we could even move it to the validator itself and make i available there. Would feel more right to me anyways.

@gidsi
Copy link
Member Author

gidsi commented Dec 7, 2019

I will open a new issue for it since it's not connected to this pull request in my mind :)

@gidsi
Copy link
Member Author

gidsi commented Dec 7, 2019

I've already removed SCSS from webpack in that branch, i forgot to fix the README /package json so it broke on your system :)

Maybe we should include a Makefile with a "run" target slightly_smiling_face

I don't know, i've used makefiles usually in a way that is now completely replaced with Dockerfiles (beside a few special cases), is there any advantage in this kinda project for makefiles?

I will open an issue for that too.

@dbrgn
Copy link
Contributor

dbrgn commented Dec 7, 2019

is there any advantage in this kinda project for makefiles?

Yes. I was used to typing lektor server to start the server. Then we introduced the webpack plugin and updated the README, but if I don't read the README then I'll still try to start the server with lektor server, which breaks the output.

If I instead only have to remember to run make server to start the server, then a PR can update the server command in the Makefile and my workflow will continue to work as before.

We could also add a target called docker_server that would automatically invoke docker to start the server container in the background.

(In a JS project I'd use a npm script in package.json instead, but Python doesn't have a default task runner.)

But that can be discussed in a separate issue (e.g. #68).

gidsi and others added 21 commits April 2, 2020 00:13
Co-Authored-By: Danilo Bargen <[email protected]>
Co-Authored-By: Danilo Bargen <[email protected]>
@dbrgn
Copy link
Contributor

dbrgn commented Apr 11, 2020

As a note already, when merging this PR, please make sure to squash all the commits, so we don't end up with dozens of single-line update commits.

@gidsi gidsi requested a review from dbrgn April 26, 2020 19:11
Copy link
Contributor

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Let's ship this! 🚀 (But again, please don't merge 60 commits consisting mostly of one-line typo fixes without squashing them first 😄 Squashing everything into one commit would be fine though if you don't have the motivation to rebase.)

@gidsi
Copy link
Member Author

gidsi commented May 10, 2020

Let's ship this! rocket (But again, please don't merge 60 commits consisting mostly of one-line typo fixes without squashing them first smile Squashing everything into one commit would be fine though if you don't have the motivation to rebase.)

Yeah, i wanted to squash everything, i don't see a lot of value in the commit messages since its basically a full rewrite.

@gidsi gidsi merged commit e326c8e into master May 25, 2020
@gidsi gidsi deleted the content_refactor branch May 25, 2020 19:10
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.

3 participants