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

Track style css #52

Merged
merged 4 commits into from
May 19, 2021
Merged

Track style css #52

merged 4 commits into from
May 19, 2021

Conversation

cryptix
Copy link
Member

@cryptix cryptix commented Mar 1, 2021

This updates #51, 100kb is an arbitrary upper bound.

I checked in a production build of style.css (13kb)

@staltz can you remind me under which circumstances we need to regenerate that file?

@staltz
Copy link
Member

staltz commented Mar 1, 2021

web/assets/style.css is:

  • in dev, a 3MB file
  • in prod, usually less than 100kB

For that reason I don't think we can put web/assets/style.css in git, because to develop the frontend we'll be needing the full 3MB file, to avoid having to generate web/assets/style.css for every (or most) template changes.

@cryptix
Copy link
Member Author

cryptix commented Mar 1, 2021

hmmm... but that means we need to run go generate before building the server for deployment.

Can't we check in the prod style.css and let frontend devs overwrite it (by running go generate -tags dev) with the big one when they want to hack in it?

@staltz
Copy link
Member

staltz commented Mar 1, 2021

I feel like this is that same discussion about putting generated code in git. Wouldn't Go 1.16 resolve this? And what about using release branches if Go 1.16 doesn't resolve it?

@cryptix
Copy link
Member Author

cryptix commented Mar 1, 2021

Now I'm confused. I thought you concern was about the diff conflicts in the generated code. The generated code is gone with 1.16.

Also I'm not proposing to commit the 3mb version ever so I'm not sure where conflicts would even come from.

@staltz
Copy link
Member

staltz commented Mar 1, 2021

Also, the prod style.css has to be regenerated sometimes depending on some changes in the templates (depending on what css classes are used).

@staltz
Copy link
Member

staltz commented Mar 1, 2021

Now I'm confused. I thought you concern was about the diff conflicts in the generated code. The generated code is gone with 1.16.

Also I'm not proposing to commit the 3mb version ever so I'm not sure where conflicts would even come from.

I was concerned about:

  • Having to resolve git conflicts
  • Making git reviews harder when there is generated code alongside source code
  • Blurring the purpose of git, which is source code version control exclusively (I think I voiced this concern in that other thread), not distribution
    • E.g. we don't commit go binaries, thus the person checking out the git repo has to have Go installed. In general if someone wants to use a git repo, they need compilers installed, and Node.js can be considered one of those compilers that generate the style.css. The user of git is considered to be a developer with the appropriate tools, not the server ops admin

@cryptix cryptix added Styling CSS for the HTML pages Go golang related stuff labels Mar 11, 2021
@cryptix cryptix added this to the Community launch milestone Apr 21, 2021
@cryptix
Copy link
Member Author

cryptix commented Apr 21, 2021

I'd like to revisit this for the community release.

Again, my main want is: At the end of a PR we make sure the production version is checked in and up2date with the used classes in the templates.

I'd accompany this with a rule for the CI which rebuilds the file and complains if there is a diff.

@staltz
Copy link
Member

staltz commented May 10, 2021

The CI job is a good idea, would be interesting to see that running in practice. I think there are different solutions to this, though, and it would be good if (tailwind) developer experience is not impacted by the addition of manual steps, if we need to go back to doing styles again. For instance, maybe the CI job could build tailwind for production and commit after merging? There's also a possibility for a CI job to automatically push to a branch, that's something I do with ssb-keys-neon actually.

@cryptix cryptix force-pushed the track-style-css branch 2 times, most recently from e0bb925 to 89958af Compare May 18, 2021 16:51
@cryptix
Copy link
Member Author

cryptix commented May 18, 2021

tested my approach and somehow it produced diffs and errors when it shouldn't have.

Now looking into the auto push if changed but getting some weird errors when trying to push to the PR. https://github.com/github-actions-x/commit/issues doesn't look super cozy either.

@cryptix
Copy link
Member Author

cryptix commented May 19, 2021

@staltz do you have insights in how this commit&push stuff works? I want it to push to the working branch but somehow it doesnt want to :-|

I see in your example it pushes to a different branch? but that seems like more hassle, too.

https://github.com/ssb-ngi-pointer/go-ssb-room/runs/2612773590

@cryptix
Copy link
Member Author

cryptix commented May 19, 2021

I just found https://github.com/marketplace/actions/pr-branch-name , maybe that helps if GITHUB_REF doesn't work.

@cryptix cryptix force-pushed the track-style-css branch from f9721dd to 429f7ed Compare May 19, 2021 09:34
@cryptix cryptix force-pushed the track-style-css branch from 86a8f8e to 6ef6972 Compare May 19, 2021 10:00
Copy link
Member

@staltz staltz left a comment

Choose a reason for hiding this comment

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

Good to go! Just remember to accept my commit suggestion :)

Co-authored-by: André Staltz <andre@staltz.com>
@cryptix cryptix merged commit b41f4e4 into master May 19, 2021
@cryptix cryptix deleted the track-style-css branch May 19, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go golang related stuff Styling CSS for the HTML pages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants