feat: allow baseElement and container in prefer-screen-queries #201
Conversation
| } | ||
| const isWithinFunction = node.init.callee.name === 'within'; | ||
| // TODO add the custom render option #198 | ||
| const usesRenderOptions = node.init.callee.name === 'render' && usesContainerOrBaseElement(node.init); |
There was a problem hiding this comment.
I left this comment as a reminder we need to address custom renders here 😅
lib/rules/prefer-screen-queries.ts
Outdated
| function usesContainerOrBaseElement(node: TSESTree.CallExpression) { | ||
| const secondArgument = node.arguments[1] | ||
| return isObjectExpression(secondArgument) && secondArgument.properties.some((property) => isProperty(property) && isIdentifier(property.key) && ALLOWED_RENDER_PROPERTIES_FOR_DESTRUCTURING.includes(property.key.name)) | ||
| } |
There was a problem hiding this comment.
Point of discussion: This should not be declared in top of the file? I don't know if we're following some pattern, but generally I prefer to put all utilities before default export
There was a problem hiding this comment.
I thought we had a linter rule for this.
There was a problem hiding this comment.
We don't have something to for this as far as I remember. In some rules is at the top, in others at the bottom.
There was a problem hiding this comment.
We don't have something to for this as far as I remember. In some rules is at the top, in others at the bottom.
Can we add this rule? or is not necessary?
There was a problem hiding this comment.
I'm not sure which ESLint rule cover this, but I guess it's better to address it on a separate ticket as it will implies fixing all reported issues (which is not directly related to this rule).
There was a problem hiding this comment.
Hmmm I really thought it was included in the recommended config, and that I moved all the utilities to the top because I had linting errors 🤔
There was a problem hiding this comment.
just in case I moved it above 😅
lib/rules/prefer-screen-queries.ts
Outdated
| function usesContainerOrBaseElement(node: TSESTree.CallExpression) { | ||
| const secondArgument = node.arguments[1] | ||
| return isObjectExpression(secondArgument) && secondArgument.properties.some((property) => isProperty(property) && isIdentifier(property.key) && ALLOWED_RENDER_PROPERTIES_FOR_DESTRUCTURING.includes(property.key.name)) | ||
| } |
There was a problem hiding this comment.
I thought we had a linter rule for this.
That's right. We will address the checking |
gndelia
force-pushed
the
prefer-screen-queries/feature/dont-warn-if-base-element-is-used
branch
from
d784a34 to
db61b10
Compare
Belco90
deleted the
prefer-screen-queries/feature/dont-warn-if-base-element-is-used
branch
|
🎉 This PR is included in version 3.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #188
One thing I noticed from this rule is that it does not check
screenis imported from RTL, nor anything about custom renders. Not sure if we should all of that for the v4 in #198 or if checking imports should be done in a separated ticket for the current version and then enhanced with custom renders for the v4... I'll assume it will be easier to do everything directly in v4 to avoid doing weird rebases but I want to know your opinion folks.For this PR I just stick with the feature requested