-
Notifications
You must be signed in to change notification settings - Fork 12
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 ESLint rule for new CSS Classes #900
Conversation
@@ -185,6 +185,7 @@ module.exports = { | |||
'@department-of-veterans-affairs/prefer-telephone-component': 1, | |||
'@department-of-veterans-affairs/telephone-contact-digits': 1, | |||
'@department-of-veterans-affairs/deprecated-classes': 1, | |||
'@department-of-veterans-affairs/use-new-utility-classes': 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhonnyoddball Do we want this set to 1 already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as recommended by default. But I will turn it off once it's installed in the vets-website
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Can we make sure that there is a follow-up ticket (or task added to a ticket) that reminds us to turn it on in vets-website when we're ready? Carol might be able to help figure out where that should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it, I'm going to remove it from the recommended so we don't have to turn it off. And when we are ready we can just upgrade to dependency and manually turn it on.
This is just in case this package is installed in another repo.
const firstErrorClass = classesArray.find(element => element.includes('vads-u')); | ||
const newClass = getNewClass(firstErrorClass); | ||
|
||
return newClass.includes('undefined') ? MESSAGE : `${MESSAGE} Try using ${newClass} instead of "${firstErrorClass}"`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit nitpicky but instead of saying "Try using" can we just say "Use the". I think we should be clear that it must be changed and not optional in our wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry. Grammatically it might be worded better with just "Use" but I'm sure you got what I meant.
@jhonnyoddball In the description of this PR, the screenshot shows it updating a color class. Does this linting rule handle colors? |
No, it just replaces the class portion. Before the |
Description
Part of department-of-veterans-affairs/vets-design-system-documentation#1343
This adds two new custom ESLint rules:
use-new-utility-classes
This Eslint rule will help VFS teams to easily update to the new CSS Class library based on USWDS v3.
This is a recommendable ESLint rule which will trigger on the pattern
vads-u
.Testing done
vets-website
Screenshots
Local testing in
vets-website
using:Acceptance criteria
Definition of done