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 reliance on tilt, grease #144

Closed
wants to merge 15 commits into from

Conversation

brendon
Copy link
Contributor

@brendon brendon commented Oct 13, 2017

Sprockets 2, 3, and hopefully 4 supported
ImportProcessor and LessTransformer rewritten to not use Tilt or Grease
Add in a DirectiveProcessor to support file include directives
Update tests to not look for Grease

Sprockets 2, 3, and hopefully 4 supported
ImportProcessor and LessTransformer rewritten to not use Tilt or Grease
Add in a DirectiveProcessor to support file include directives
Update tests to not look for Grease
@brendon
Copy link
Contributor Author

brendon commented Oct 13, 2017

@simi, my major motivation is to try and get Semantic-UI-Rails-LESS to work with this new version of less-rails. So far it's not working very well with the following errors:

couldn't find file 'semantic_ui/definitions/modules/accordion.less' with type 'text/css'

etc... I managed to get past that last night by adding the Sprockets::DirectiveProcessor but now that doesn't work again. Perhaps we should add some local tests that test sprockets directives?

@simi
Copy link
Collaborator

simi commented Oct 13, 2017

Failing spec on current master and fixed by this will be awesome. I'll try to take a look at this also.

@brendon
Copy link
Contributor Author

brendon commented Oct 13, 2017

I forgot to send up the new travis.yml but have done that now. The spec failure is that jruby isn't installing at all. Might be a problem on the travis side?

I've managed to fix that other Semantic-UI-Rails-LESS error by removing the .less extensions from the requires:

 *= require semantic_ui/definitions/modules/accordion.less

becomes

 *= require semantic_ui/definitions/modules/accordion

which seems to be the standard anyway.

Now I have another failure but it's around variable overrides by the look:

SemanticUi::InstallGenerator / application.css / override variables / in theme.config / should include changed cached variable

@brendon
Copy link
Contributor Author

brendon commented Oct 13, 2017

This is why jruby is failing: jruby/jruby#4789

I'll see if we can just mark it as a tolerated failure for now. I'll also not run the sprockets2 test against Ruby 2.4 since Rails 4.0 doesn't like that.

@brendon
Copy link
Contributor Author

brendon commented Oct 14, 2017

Added a test to check that recompilation occurs within the same test case if a dependency or sub dependency changes. I think this is basically all good to go now @simi.

@brendon
Copy link
Contributor Author

brendon commented Oct 19, 2017

Hi @simi, this is complete now. I've also got Semantic-Ui-Rails-LESS working with this branch too. Are you able to look into merging this and doing a new release?

The only change I had to make to Semantic-Ui-Rails-LESS to get tests to pass with this branch was:

 *= require semantic_ui/definitions/modules/accordion.less

became:

 *= require semantic_ui/definitions/modules/accordion

I'm not sure that extensions there were ever actually a thing, so perhaps this was a working fault in the past?

@brendon
Copy link
Contributor Author

brendon commented Oct 26, 2017

Hi @simi, could you cause a rebuild of the current commit on this branch with travis? I think the failure was intermittent by the looks?

1 similar comment
@brendon
Copy link
Contributor Author

brendon commented Oct 26, 2017

Hi @simi, could you cause a rebuild of the current commit on this branch with travis? I think the failure was intermittent by the looks?

@brendon
Copy link
Contributor Author

brendon commented Oct 26, 2017

Oh that's weird, it just did it itself...

@simi
Copy link
Collaborator

simi commented Oct 26, 2017

It was triggered by me @brendon.

@brendon
Copy link
Contributor Author

brendon commented Oct 26, 2017

Oh! woah, github weirdness. Didn't notice I submitted the message twice! I saw it in the text box and assume I hadn't sent it yet :)

@brendon
Copy link
Contributor Author

brendon commented Oct 27, 2017

Looks like the tests are good @simi. Can you merge this and release a new version? It might pay even to yank 3.0.0 as it's not sprockets 2 compatible, and is also probably buggy.

@brendon
Copy link
Contributor Author

brendon commented Jan 23, 2018

Hi @simi, could you take another look at this? It's ready to go as far as I know.

@brendon
Copy link
Contributor Author

brendon commented Mar 19, 2018

Hi @simi, I think we should get this one merged and signed off :) Let me know if you have any queries on the code :)

@simi
Copy link
Collaborator

simi commented Aug 23, 2018

Hello @brendon. I think we will need to release this as new major version. Can you please add also version change, changelog entry and maybe readme note?

I was also thinking to drop support for older versions in new major version. What do you think @brendon?

@brendon
Copy link
Contributor Author

brendon commented Aug 23, 2018

Hi @simi, probably due to the need to remove the .less extension from the require statements, yes a new major version should be released. I'll update this now.

Sprockets 4 still hasn't been released yet. I'm more in favour of keeping 2 and 3 support in this next version. The work has already been done, so it'd be more work to remove support.

Changelog and Readme updated
@brendon
Copy link
Contributor Author

brendon commented Aug 23, 2018

@simi, I've just done the work you requested. If you could review and publish that would be great. I can then work on the Semantic-UI dependency update.

@metaskills, given it's your repo, would you be able to add me as a maintainer and gem author to help alleviate the burden on @simi?

@brendon
Copy link
Contributor Author

brendon commented Aug 24, 2018

Hi @simi, tests are green, so we're good to go.

@simi
Copy link
Collaborator

simi commented Aug 24, 2018

@brendon I'll release rc version of 4.0 and ping you back.

@simi
Copy link
Collaborator

simi commented Aug 24, 2018

I have created v4.0 branch and released rc1 version (https://rubygems.org/gems/less-rails/versions/4.0.0.rc1). Please everyone interested test and report back.

@brendon
Copy link
Contributor Author

brendon commented Aug 24, 2018

Thanks @simi, I'll add a note over in the issue.

@simi
Copy link
Collaborator

simi commented Aug 31, 2018

Hello @brendon how is testing of rc going? Any problems?

@brendon
Copy link
Contributor Author

brendon commented Aug 31, 2018

Hi @simi, see: Semantic-Org/Semantic-UI-Rails-LESS#49

I'm happy to proceed given that library's tests are green. Let's push out 4.0.0, and then can you also merge Semantic-Org/Semantic-UI-Rails-LESS#49 for me?

No one else has come back with any problems.

@brendon
Copy link
Contributor Author

brendon commented Oct 31, 2018

Hi @simi, how come the PR didn't get merged? Did you do it just using git? :)

@simi
Copy link
Collaborator

simi commented Oct 31, 2018

Yeah, I did it manually since I had to create v4.0 branch for rc first.

@brendon brendon closed this Oct 31, 2018
@brendon
Copy link
Contributor Author

brendon commented Oct 31, 2018

All good. I'll close this then :)

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