Sprockets to propshaft #1410
Sprockets to propshaft #1410
Conversation
|
I don’t want to split up the css into multiple files. I don’t see any benefit for a site this small. Also, the |
|
The reason I suggested breaking up application.css to make the css more modular and with http 2, it is supported without any performance penalty. But i get it that benefits are mostly cosmetic and css file is not that too large. A side question, what do you think about incorporating tailwindcss into the project ? It would be a larger rewrite of the css, but may make the UI more "modern".. maybe ? :D |
|
Tailwind would reintroduce the nodejs dep I want to remove (or other build complexity). It would add a new, big dependency to maintain and for potential contributors to learn. It's not really a goal to keep the site looking modern, I want to keep css small so we can run it on cheap hosting and pages load in a snap. And because we're a programming community, our users are unusually broad in choice of browsers so it's nice to keep things simple and reliable. If you want a big css project, #1279 has one. But let's keep it separate from this PR! |
|
I can't find the original article on reducing duplication in the dark and light versions of the CSS but this article on light-dark has a design pattern that looks great. Probably also worth putting in a different PR than this build update! |
|
I was looking at the autosize.js javascript library used in the project and was wondering if is possible to replace these legacy libraries with something simple in modern html and js features. Will something like this capture all autosize.js functionalies. |
|
So, I don’t think I’ve looked at that autosize js for years, I’m glad you took a fresh look at it. Reviewing, I think you’re right, we could drop that dependency and have just a little custom code for it. It looks like you’ll need to add If you want to continue removing the library, please open a separate PR for removing it. I really look forward to it! |
app/javascript/application.js
Outdated
| @@ -93,11 +95,11 @@ class _LobstersFunction { | |||
| this.curUser = null; | |||
|
|
|||
| this.storyFlagReasons = ({ | |||
| <%= Vote::STORY_REASONS.map{|k,v| "'#{k}': '#{v}'" }.join(", ") %> | |||
| 'O': 'Off-topic', 'A': 'Already Posted', 'B': 'Broken Link', 'S': 'Spam', '': 'Cancel' | |||
There was a problem hiding this comment.
This (and commentFlagReasons) exist to avoid duplicating flags between the Vote model and here. Can you find another way to remove this duplication? Is there any negative to doing this with ERB?
There was a problem hiding this comment.
If we use ERB template for css and javascript, we will need to have sprocket. propshaft and importmap doesn't support erb templating. They don''t process files in any way. simply serves to browser after appending a checksum hash. This simplifies our asset pipeline, but we lose some features. That's why I had to remove templating from css and javascript files.
To avoid duplication, we could put this map in a view helper module and render that in the html directly and read it in javascript from html, if need be. Does that sound like a good idea ?
There was a problem hiding this comment.
Ah, thanks for explaining.
It was already a project to remove inline scripts from the site (#696 #697 #779) as a long-term security improvement, so I don't want to add one back.
One option would be to have the js be the authoritative list of flag reasons and have Rails parse it out at startup. Maybe it's because I'm mostly a backend developer, but that feels backwards and I don't want to do that.
I guess we only add and remove flags very infrequently. Please add a comment to this js explaining the reasons are duplicated in the Vote model, and add the reverse comment to Vote model.
There was a problem hiding this comment.
Added the comments.
There was a problem hiding this comment.
I talked this through on stream and someone pointed out that I misunderstood your suggestion. An easy way to remove this duplication would be to have an invisible html element with the flags in json and have the js load them. Now that I understand that, I think that's the best idea. Sorry I had you write comments.
We don't need to make it a standalone helper please add a meta tag in the head.
So I think that'll be <meta name="storyFlags" content="<%= Vote::STORY_REASONS.to_json "> and something very similar for commentFlags.
There was a problem hiding this comment.
I spent close to 2 hours making test work, but rspec is new to me. Will spend some more time later.
Overview
This PR uses propshaft in place of sprockets for asset management and delivery. This is in line with rails 8's recommendation and as outlines in this issue #1366
This PR
Testing
lobsters_autosize.mp4
lobsters_tomselect.mp4