Skip to content

Conversation

@mao-sz
Copy link
Contributor

@mao-sz mao-sz commented Dec 11, 2025

Because

The current lint script runs only in app/javascript which therefore misses the JS config files in the repo root. Can't see a reason for these to not be linted either.

The current stylelint script also misses application.tailwind.css.

This PR

  • Amends paths for lint and stylelint scripts
    • Removes --verbose flag for standard as it isn't a valid flag.
  • Amends stylelint config with Tailwind at-rules and import-notation (for consistency within file)
  • Fixes JS and CSS lint errors in missed files.

Issue

Closes #XXXXX

Additional Information

Opening up the standard path revealed JS controllers in

Is this intentional? I thought they would go in app/javascript/controllers.

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
    • Feature - adds new or amends existing user-facing behavior
    • Chore - changes that have no user-facing value, refactors, dependency bumps, etc
    • Fix - bug fixes
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • I have verified all tests and linters pass after making these changes.
  • If this PR addresses an open issue, it is linked in the Issue section
  • If applicable, this PR includes new or updated automated tests

--verbose not a valid flag for standardjs

Original paths did not cover all JS/CSS files.
Now caught by stylelint's new path.
`import-notation: url` is the default, but url() in @import is optional.
Appeasing the linter for this would make things inconsistent with paths
in at-rules like @config, which stylelint won't be able to lint for
url(), so it's more sensible to configure it for `string`.
Covers files not caught by original `app/javascript` lint path
@github-project-automation github-project-automation bot moved this to 📋 Backlog / Ideas in Main Site Dec 11, 2025
Copy link
Member

@KevinMulhern KevinMulhern left a comment

Choose a reason for hiding this comment

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

Great stuff! thanks @mao-sz

@github-project-automation github-project-automation bot moved this from 📋 Backlog / Ideas to 👀 In review in Main Site Jan 6, 2026
@mao-sz
Copy link
Contributor Author

mao-sz commented Jan 6, 2026

Cheers @KevinMulhern
Thoughts on the 2 JS controllers bit in the additional info part of the PR form? If action is required then I can PR separately

@mao-sz mao-sz merged commit f5f7c17 into TheOdinProject:main Jan 6, 2026
5 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Main Site Jan 6, 2026
@mao-sz mao-sz deleted the linters branch January 6, 2026 21:00
@KevinMulhern
Copy link
Member

Sorry I missed that @mao-sz. Yep, those are intentional.

Ideally we’d only have generic controllers that can be used anywhere in the app, living in app/javascript/controllers. In practice though, that’s easier said than done, and I think we have a few controllers there with fairly narrow use cases.

View components make it easier though. They allow Stimulus controllers to live in a component’s sidecar when the behavior is tightly coupled to that component. It's mostly a DX win - everything related to the component lives in one place, so there’s less context switching - but it also clearly communicates that the controller is not meant to be reused elsewhere.

@mao-sz
Copy link
Contributor Author

mao-sz commented Jan 6, 2026

Gotcha 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants