Skip to content

Conversation

@stefannibrasil
Copy link
Contributor

I will rebase this branch once #22 is merged.

I ran into an issue when registering the controller
but when changing how ApplicationController
was being imported fixed the issue. Please let me
know what I am missing.

Besides those changes, I am also adding a check in
for folks to verify their changes are working as expected
before moving on to the next lesson.

Instead of adding a brewfile, we check
if the vips package is not installed. Then,
the user decides how they want to install it.
We're not using them for any purpose, so let's
remove them.
Adding some suggestions, feel free to ignore them.
The docs say we shouldn't need to add a ruby
version here if we have a `ruby-version` file.
When initiating the app for the first time,
and accessing localhost:3000, it can be confusing
to get errors about episodes not being found, or
their images not being found.

This happens because in the setup, background jobs
to download the audio and images are scheduled.
When we run `./bin/dev`, the background jobs
will get executed. This will take some time, so
I thought of adding a note to users.
Error: This request has been automatically failed because it uses a deprecated version of `actions/upload-artifact: v2`.
Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/
CI already install libvips, so we don't need to worry about
it here.

linter
I ran into an issue when registering the controller
but when changing how ApplicationController
was being imported fixed the issue. Please let me
know what I am missing.

Besides those changes, I am also adding a check in
for folks to verify their changes are working as expected
before moving on to the next lesson.

- run `./bin/rails test` to verify the tests pass
- press ⌘ k anywhere in the episodes list page to verify the search shortcut works as expected
note: using `Control` instead of ⌘ should work as well for non-Mac users
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this based on github/hotkey#66 However, I have not tested in a non-Mac machine. I guess we can leave this and ask someone later to try it?

```js
// app/javascript/controllers/hotkey_controller.js
import ApplicationController from "controllers/application_controller"
import { Controller } from '@hotwired/stimulus'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still don't know why this was needed. In lesson 2, this controllers inherits from ApplicationController and it works 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Were you following the existing guidance in the lesson plan by running ./ta/start-lesson 1 and then attempting to get tests to pass? Did you create an empty app/javascript/controllers/application_controller.js file?

// app/javascript/controllers/application_controller.js
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
}

I think the short answer is that since we plan on moving away from Tutorial Assistant for this initial release, this shouldn't matter.

Instead, I think we need to work on our messaging and setup instructions to demonstrate how the app is built up from a Turbo-less Rails app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did follow the lesson plan but did not create an empty controller file. I've added the message about the turbo-less state in the first PR 👍🏼

A follow up question: without the teaching assistant, are you planning on removing the solutions from main?

@stefannibrasil
Copy link
Contributor Author

I am going to close this one because the only change from this branch is this commit: 9365826 that I extracted to #22

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.

3 participants