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

Remove Sprockets dependency #48

Merged
merged 1 commit into from
Oct 10, 2020
Merged

Remove Sprockets dependency #48

merged 1 commit into from
Oct 10, 2020

Conversation

barrywoolgar
Copy link
Contributor

@barrywoolgar barrywoolgar commented Oct 4, 2020

Hello @markets, this is the long overdue PR in response to #46 - sorry for the delay.

This resolves the basic problem of a Rails app not being able to boot after adding the gem to the project if Sprockets is not enabled. The README has been updated to describe the problem and provide instructions for a workaround.

Now in the case of Sprockets being disabled:

  • An error message is displayed to the user when the installer is run, providing a link to more information
  • The installer supports a --skip-sprockets to bypass this error, so they can implement the workaround for CSS styles
  • The images (logo, globe, paperclip) are replaced with unicode alternatives

The use of emoji might offend, especially with regard to the logo, so I'm very open to suggestions here. Perhaps they should be inlined using Base64 data URLs instead?

Thanks

@barrywoolgar
Copy link
Contributor Author

Here's a partial screenshot of the image replacements:

Screenshot

@markets
Copy link
Owner

markets commented Oct 5, 2020

Hi @barrywoolgar no worries! thanks a lot for working on this 👍🏼

The use of emoji might offend, especially with regard to the logo, so I'm very open to suggestions here. Perhaps they should be inlined using Base64 data URLs instead?

Yeah, maybe that's a good solution, so it will work for both scenarios with no extra logic.

@barrywoolgar barrywoolgar marked this pull request as draft October 6, 2020 08:18
@barrywoolgar
Copy link
Contributor Author

No problem, I only thought of the Base64 idea when I was writing the PR, otherwise I would have done it that way to start with!

@barrywoolgar
Copy link
Contributor Author

OK, I've switched to Base64 encoded data URIs and that's appears to be working well.

I've also added a method to the installer that will automatically copy Maily's application.html.erb file into the right place in the user's app. This removes an unnecessary manual step from the workaround - now they just need to edit it and replace the stylesheet_link_tag with their preferred method for replacing the styles. The README has been updated accordingly.

@barrywoolgar
Copy link
Contributor Author

barrywoolgar commented Oct 8, 2020

This leaves only the use of SCSS relying on Sprockets. It's tempting to try and resolve that in this PR too, but perhaps that should be a separate undertaking?

(Edited to remove some too-early-in-the-morning lack of thinking)

@markets
Copy link
Owner

markets commented Oct 8, 2020

hi @barrywoolgar 👋🏼

This leaves only the use of SCSS relying on Sprockets. It's tempting to try and resolve that in this PR too

That would be amazing, I think it makes sense to do it here in the same PR, so we can ship a new version compatible with both engines, with no extra steps or workarounds 👌🏼

We can just use the simple approach we took with the js: move to plain css <style>, in a partial https://github.com/markets/maily/blob/master/app/views/maily/shared/_javascript.html.erb. We can use css variables for current sass vars.

This will close #47 and also will allow us to remove this "hack" 👇🏼 too (as we wont need this dep anymore 🎉 )

maily/maily.gemspec

Lines 18 to 22 in 6d1caef

if RUBY_VERSION >= "2.5.0"
s.add_dependency "sassc-rails"
else
s.add_dependency "sass-rails", ">= 4", "< 6"
end

After this change, it would be a good time to cut a v1.0 :)

…ting SCSS to CSS

This now makes Maily compatible with Webpacker/API projects by
not requiring Sprockets be loaded in the parent application.
@barrywoolgar
Copy link
Contributor Author

OK, that negates quite a few of the changes I originally made but that can only be a good thing!

I have elected to use ERB variables instead of CSS variables as a final mercy to IE users.

I have also removed the functionally-orphaned SCSS file completely as that will likely only cause confusion at a later date. It's easy enough for gem developers to convert back and forth if and when necessary. In future a proper pipeline can be established once we know what that should look like...

@markets
Copy link
Owner

markets commented Oct 9, 2020

Thank you so much @barrywoolgar! I just made a first quick review and everything seems fine ✅ to me. I'll make a second pass this weekend and merge it!

@markets markets merged commit 93121f0 into markets:master Oct 10, 2020
@markets
Copy link
Owner

markets commented Oct 10, 2020

merged 🚀 @barrywoolgar less deps, less lines, simpler approach, more "compatible" 👌🏼 nice pull request! I'll make some tweaks and prepare new release. Thanks!

@markets markets changed the title Warn about missing Sprockets, support a workaround Remove Sprockets dependency Oct 10, 2020
@markets
Copy link
Owner

markets commented Oct 10, 2020

@barrywoolgar v1.0.0 pushed 🚀 to RubyGems:

Thanks a lot for working on this and making Maily simpler 👏🏼

@barrywoolgar
Copy link
Contributor Author

Hurray! No problem at all, thanks for your patience while I took the long way round :)

Congrats on v1... now, what's for v2?

@markets
Copy link
Owner

markets commented Oct 10, 2020

👍🏼 👍🏼

My vague thoughts for v2 😄 :

@barrywoolgar
Copy link
Contributor Author

Those all sound very good!

I've got a patch that applies Premailer if it's available, so the email markup is closer to what is sent when using it (ie. lots of inline styles), and another that handles namespaced mailers a bit better when listing them in the sidebar.

I need to catch up on some things in Real Life first though, but I'll generate some PRs when I get a bit of free time :)

All the best!

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