Skip to content

Conversation

@kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Dec 16, 2022

Closes #146

This codemod removes the redundant isSVG flag now that the default is true.

I tried to get the codemod to throw a warning for all Spinner imports but I haven't quite figured out how to modify the severity of the message (which are all thrown as errors) so I'm holding off on that for now while I tinker with it more locally.

@gitdallas
Copy link
Contributor

i'm curious if we really just want to remove the property. Could this negatively impact people who are using isSVG=false explicitly already?

Copy link
Collaborator

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

In regards to @gitdallas comment above, maybe rather than removing the prop we just warn consumers that the default value has been updated? Or tweaking the message for the fix to mention something like, "If you are passing false to this prop, you can ignore this rule".

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jan 11, 2023

Yeah the warning bit is what I'm a bit lost on. I can't figure out how to lessen the severity from a full error. I agree that maybe removing the prop isn't necessary here so I'll take another look and remove that.

@gitdallas
Copy link
Contributor

gitdallas commented Jan 11, 2023

this pr has an example that could help adding some logic to look at the value and determine what to do based on that #205

i think the rules should be:

  • isSVG or isSVG={true} would be removed
  • anything else just leave alone, no context.report()

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jan 12, 2023

Agreed with those rules. Added in some extra tests to ensure that isSVG={true} gets removed and isSVG={false} remains as is.

Copy link
Contributor

@gitdallas gitdallas left a comment

Choose a reason for hiding this comment

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

couple comments


```jsx
<Card />
<Card />
Copy link
Contributor

Choose a reason for hiding this comment

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

prettier ran?

Copy link
Contributor Author

@kmcfaul kmcfaul Jan 13, 2023

Choose a reason for hiding this comment

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

Must have, I'll go back and add spaces. I think maybe it ran when I was doing the merge resolutions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave in the single quotes changing to double quotes because that's what the JSX should look like

@gitdallas gitdallas merged commit bb6522b into main Jan 13, 2023
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.

Spinner - warn isSVG prop default value now true

4 participants