Skip to content

LG-3480: Add ERBLint to forbid deprecated classes#4599

Merged
aduth merged 3 commits intomasterfrom
aduth-erb-lint
Jan 22, 2021
Merged

LG-3480: Add ERBLint to forbid deprecated classes#4599
aduth merged 3 commits intomasterfrom
aduth-erb-lint

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 21, 2021

Why: As a developer, I expect that static analysis tooling will prevent me from adding deprecated classes, so that I'm not contributing against the effort to transition from BassCSS to USWDS.

Includes classes marked in LG-3261 technical research as not being in use, plus alignment classes migrated as of LG-3484 (#4540).

An important caveat is that ERBLint is only able to detect classes assigned through a class="..." HTML assignment, and not via a Rails tag helper (e.g. image_tag(..., class: '...')).

aduth added 2 commits January 21, 2021 13:50
**Why**: As a developer, I expect that static analysis tooling will prevent me from adding deprecated classes, so that I'm not contributing against the effort to transition from BassCSS to USWDS.
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, for the tag helper maybe we should add something that runs in specs, that overrides the actionview tag method (or whatever the base one that they all go through is) and warns/errors if it sees a banned classname passed?

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

This is great 👍🏼

@aduth
Copy link
Contributor Author

aduth commented Jan 22, 2021

for the tag helper maybe we should add something that runs in specs, that overrides the actionview tag method (or whatever the base one that they all go through is) and warns/errors if it sees a banned classname passed?

I like this idea. Incidentally I happened to be looking at the other ticket in this sprint LG-3483 "Implement deprecated class matchers to fail in test specs" trying to recall how it differed from this one. I believe the intention with the other ticket is similar to what you're describing, i.e. incorporate deprecated class detection in the test specs. As originally written, I think it was expected more as hooking in to render or Capybara::Session.visit, but I think the tag method approach could work just as well too (probably a fair bit more efficient as well).

@aduth aduth merged commit 52983f8 into master Jan 22, 2021
@aduth aduth deleted the aduth-erb-lint branch January 22, 2021 15:41
aduth added a commit that referenced this pull request Jan 25, 2021
**Why**: As a developer, I expect that RSpec automated render testing will prevent me from adding deprecated classes, so that I'm not working against the effort to transition from BassCSS to USWDS.

Implemented using RSpec mock expecting any occurrence of tag builder to not be invoked using a `class` option including one of the classes configured in ERBLint as deprecated (see #4599).
aduth added a commit that referenced this pull request Jan 25, 2021
* LG-3483: Fail tests when using tag helper with deprecated class

**Why**: As a developer, I expect that RSpec automated render testing will prevent me from adding deprecated classes, so that I'm not working against the effort to transition from BassCSS to USWDS.

Implemented using RSpec mock expecting any occurrence of tag builder to not be invoked using a `class` option including one of the classes configured in ERBLint as deprecated (see #4599).

* Monkey-patch TagBuilder#tag_option for deprecated class detection

**Why**: Better error messaging, better reconciliation of class defined in alternative formats (array, hash)

Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>

Co-authored-by: Zach Margolis <zbmargolis@gmail.com>
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