-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat(DrawerContent): removed no-background colorVariant #629
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
### drawerContent-replace-noBackground-colorVariant [(#10211)](https://github.com/patternfly/patternfly-react/pull/10211) | ||
|
||
The "no-background" value of the `colorVariant` prop on DrawerContent has been removed, and a new "primary" value has been added. | ||
|
||
Additionally, a new DrawerContentColorVariant enum has been added and should be used instead of the DrawerColorVariant enum. The fix when the DrawerColorVariant enum is being used will replace the `colorVariant` prop value with a string. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note, the logic in the rule for this part will cause the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am OK with this rule updating the |
||
|
||
#### Examples | ||
|
||
In: | ||
|
||
```jsx | ||
%inputExample% | ||
``` | ||
|
||
Out: | ||
|
||
```jsx | ||
%outputExample% | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
const ruleTester = require("../../ruletester"); | ||
import * as rule from "./drawerContent-replace-noBackground-colorVariant"; | ||
|
||
ruleTester.run("drawerContent-replace-noBackground-colorVariant", rule, { | ||
valid: [ | ||
{ | ||
code: `<DrawerContent colorVariant="no-background" />`, | ||
}, | ||
{ | ||
code: `<DrawerContent colorVariant={DrawerColorVariant.primary} />`, | ||
}, | ||
{ | ||
code: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent colorVariant="primary" />`, | ||
}, | ||
{ | ||
code: `import { DrawerContent, DrawerContentColorVariant } from '@patternfly/react-core'; <DrawerContent colorVariant={DrawerContentColorVariant.primary} />`, | ||
}, | ||
{ | ||
code: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent colorVariant={DrawerColorVariant.primary} />`, | ||
}, | ||
{ | ||
code: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent someOtherProp />`, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent colorVariant="no-background" />`, | ||
output: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent />`, | ||
errors: [ | ||
{ | ||
message: `The "no-background" value of the \`colorVariant\` prop on DrawerContent has been removed.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent colorVariant={"no-background"} />`, | ||
output: `import { DrawerContent } from '@patternfly/react-core'; <DrawerContent />`, | ||
errors: [ | ||
{ | ||
message: `The "no-background" value of the \`colorVariant\` prop on DrawerContent has been removed.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { DrawerContent } from '@patternfly/react-core'; const color = "no-background"; <DrawerContent colorVariant={color} />`, | ||
output: `import { DrawerContent } from '@patternfly/react-core'; const color = "no-background"; <DrawerContent />`, | ||
errors: [ | ||
{ | ||
message: `The "no-background" value of the \`colorVariant\` prop on DrawerContent has been removed.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; <DrawerContent colorVariant={DrawerColorVariant.default} />`, | ||
output: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; <DrawerContent colorVariant="default" />`, | ||
errors: [ | ||
{ | ||
message: `The DrawerContentColorVariant enum should be used instead of the DrawerColorVariant enum when passed to the DrawerContent component. This fix will replace the colorVariant prop value with a string.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; <DrawerContent colorVariant={DrawerColorVariant.noBackground} />`, | ||
output: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; <DrawerContent />`, | ||
errors: [ | ||
{ | ||
message: `The "no-background" value of the \`colorVariant\` prop on DrawerContent has been removed.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; const color = DrawerColorVariant.default; <DrawerContent colorVariant={DrawerColorVariant.default} />`, | ||
output: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; const color = DrawerColorVariant.default; <DrawerContent colorVariant="default" />`, | ||
errors: [ | ||
{ | ||
message: `The DrawerContentColorVariant enum should be used instead of the DrawerColorVariant enum when passed to the DrawerContent component. This fix will replace the colorVariant prop value with a string.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; const color = DrawerColorVariant.noBackground; <DrawerContent colorVariant={color} />`, | ||
output: `import { DrawerContent, DrawerColorVariant } from '@patternfly/react-core'; const color = DrawerColorVariant.noBackground; <DrawerContent />`, | ||
errors: [ | ||
{ | ||
message: `The "no-background" value of the \`colorVariant\` prop on DrawerContent has been removed.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
import { Rule } from "eslint"; | ||
import { JSXOpeningElement } from "estree-jsx"; | ||
import { getFromPackage, getAttribute, getAttributeValue } from "../../helpers"; | ||
|
||
// https://github.com/patternfly/patternfly-react/pull/10211 | ||
module.exports = { | ||
meta: { fixable: "code" }, | ||
create: function (context: Rule.RuleContext) { | ||
const { imports } = getFromPackage(context, "@patternfly/react-core"); | ||
|
||
const drawerContentImport = imports.find( | ||
(specifier) => specifier.imported.name === "DrawerContent" | ||
); | ||
const drawerColorVariantEnumImport = imports.find( | ||
(specifier) => specifier.imported.name === "DrawerColorVariant" | ||
); | ||
const validDrawerContentValues = ["default", "primary", "secondary"]; | ||
|
||
return !drawerContentImport | ||
? {} | ||
: { | ||
JSXOpeningElement(node: JSXOpeningElement) { | ||
if ( | ||
!( | ||
node.name.type === "JSXIdentifier" && | ||
drawerContentImport.local.name === node.name.name | ||
) | ||
) { | ||
return; | ||
} | ||
const colorVariantProp = getAttribute(node, "colorVariant"); | ||
|
||
if (!colorVariantProp) { | ||
return; | ||
} | ||
|
||
const colorVariantValue = getAttributeValue( | ||
context, | ||
colorVariantProp.value | ||
); | ||
const drawerColorVariantLocalName = | ||
drawerColorVariantEnumImport && | ||
drawerColorVariantEnumImport.local.name; | ||
const hasPatternFlyEnum = | ||
colorVariantValue && | ||
colorVariantValue.object && | ||
colorVariantValue.object.name === drawerColorVariantLocalName; | ||
const hasNoBackgroundValue = | ||
colorVariantValue && colorVariantValue.property | ||
? hasPatternFlyEnum && | ||
colorVariantValue.property.name === "noBackground" | ||
: colorVariantValue === "no-background"; | ||
|
||
if (!hasPatternFlyEnum && !hasNoBackgroundValue) { | ||
return; | ||
} | ||
|
||
const message = hasNoBackgroundValue | ||
? 'The "no-background" value of the `colorVariant` prop on DrawerContent has been removed.' | ||
: "The DrawerContentColorVariant enum should be used instead of the DrawerColorVariant enum when passed to the DrawerContent component. This fix will replace the colorVariant prop value with a string."; | ||
context.report({ | ||
node, | ||
message, | ||
fix(fixer) { | ||
const fixes = []; | ||
if (hasNoBackgroundValue) { | ||
fixes.push(fixer.replaceText(colorVariantProp, "")); | ||
} | ||
|
||
if (!hasNoBackgroundValue && hasPatternFlyEnum) { | ||
const enumPropertyName = colorVariantValue.property.name; | ||
fixes.push( | ||
fixer.replaceText( | ||
colorVariantProp, | ||
validDrawerContentValues.includes(enumPropertyName) | ||
? `colorVariant="${colorVariantValue.property.name}"` | ||
: "" | ||
) | ||
); | ||
} | ||
return fixes; | ||
}, | ||
}); | ||
}, | ||
}; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { DrawerContent, DrawerColorVariant } from "@patternfly/react-core"; | ||
|
||
export const DrawerContentReplaceNoBackgroundColorVariantInput = () => { | ||
const stringColor = "no-background"; | ||
const enumColor = DrawerColorVariant.default; | ||
|
||
return ( | ||
<> | ||
<DrawerContent colorVariant='no-background' /> | ||
<DrawerContent colorVariant={DrawerColorVariant.default} /> | ||
<DrawerContent colorVariant={stringColor} /> | ||
<DrawerContent colorVariant={enumColor} /> | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { DrawerContent, DrawerColorVariant } from "@patternfly/react-core"; | ||
|
||
export const DrawerContentReplaceNoBackgroundColorVariantInput = () => { | ||
const stringColor = "no-background"; | ||
const enumColor = DrawerColorVariant.default; | ||
|
||
return ( | ||
<> | ||
<DrawerContent /> | ||
<DrawerContent colorVariant="default" /> | ||
<DrawerContent /> | ||
<DrawerContent colorVariant="default" /> | ||
</> | ||
); | ||
}; |
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.
Can we check that the type of
variableDeclaration.defs[0]
equals to"Variable"
here? It will then also make the type ofvariableInit
not beingany
, because thenode
will be treated asnode: VariableDeclarator
.EDIT: after checking it further, it will create some type problems when accessing
colorVariantValue.object
orcolorVariantValue.property
, because the colorVariantValue won't have the value ofany
anymore. So some additional functionality for checking if the return value is of type MemberExpression would be needed.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.
We can open a followup to address this if you'd like.
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.
Sure. It probably won't cause any problems for now, since I cannot imagine someone would be passing a function name (or anything other than a variable name) as an attribute value.
Opened a followup issue: #630