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

Add hotwire-livereload #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add hotwire-livereload #164

wants to merge 1 commit into from

Conversation

ajdubovoy
Copy link
Collaborator

@ajdubovoy ajdubovoy commented Apr 3, 2024

This is optional and I would totally understand if we reject it because we don't want to add another dependency.

But this PR would add livereload functionality to our Rails templates. I've just tested it and it works almost instantly for CSS and JS changes. This is a feature that we used to have for JS with Webpack but lost with the new importmap setup. We've actually never had it for CSS, so it's even fancier than before (yay!).

I thought it would be a nice thing to include since it could speed up development for students during project weeks and also help with teachers doing CSS changes in lectures.

I'm honestly not sure we need to explain this to students, but if we wanted to, I could add it into our "front-end setup" guide, which is done in some livecodes, or into the Rails Assets lecture. But I think it's such a simple change (literally adding one gem and running a Terminal command) that it's almost not worth the airtime.

Related:

This is _optional_ and I would totally understand if we reject it
because we don't want to add another dependency.

But this PR would add livereload functionality to our Rails templates.
I've just tested it and it works almost instantly for CSS and JS
changes. This is a feature that we used to have for JS with Webpack but
lost with the new importmap setup. We've actually never had it for CSS,
so it's even fancier than before (yay!).

I thought it would be a nice thing to include since it could speed up
development for students during project weeks and also help with
teachers doing CSS changes in lectures.

I'm honestly not sure we need to explain this to students, but if we
wanted to, I could add it into our "front-end setup" guide, which is
done in some livecodes, or into the Rails Assets lecture. But I think
it's such a simple change (literally adding one gem and running a
Terminal command) that it's almost not worth the airtime.
@ajdubovoy ajdubovoy requested review from Eschults and juliends April 3, 2024 12:39
Copy link
Contributor

@juliends juliends left a comment

Choose a reason for hiding this comment

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

Hey @ajdubovoy 👋

Personally it's a feature I have appreciated in the past when working with students on project weeks.
If we chose to add it we might need a dedicated slide in the course the first time we will actually use it.

Additionally it can be easily disabled 👌

@ajdubovoy
Copy link
Collaborator Author

Ok! Maybe the best place to put it would be in the Rails Assets lecture? That way we could explain like "This is what SCSS is...also install this gem and it'll live reload". And yes, I noticed that Terminal command and think it'd be nice to mention in the slide that it can be enabled/disabled

Copy link
Member

@Eschults Eschults left a comment

Choose a reason for hiding this comment

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

Nice addition, indeed the Rails Assets lecture would be the right place to mention it in the slides ✅

@ajdubovoy
Copy link
Collaborator Author

Added in https://github.com/lewagon/karr/pull/1043.

Good to merge?~!

Copy link

@YukinagaS YukinagaS left a comment

Choose a reason for hiding this comment

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

Amazing!

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.

4 participants