Skip to content

Add $ to columnCount prop used by styled component#2992

Merged
robertbrignull merged 1 commit intomainfrom
robertbrignull/columnCount
Oct 18, 2023
Merged

Add $ to columnCount prop used by styled component#2992
robertbrignull merged 1 commit intomainfrom
robertbrignull/columnCount

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This stops the following error from appearing during tests:
Screenshot 2023-10-18 at 11 50 08

I'm not totally sure if this has an effect on behaviour in production. Perhaps it was working but just with a warning.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team as a code owner October 18, 2023 10:55

type TableContainerProps = {
columnCount: number;
$columnCount: number;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See https://styled-components.com/docs/basics#attaching-additional-props for docs on props for styled components.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks, I didn't know about this feature. Apparently, these are transient props. I think it's fine to use these when we're only using the prop in the same file, but for components that are used outside of the same file, we should probably not use those. That way we can keep the implementation separate from the exposed interface.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

Interesting. Thanks for finding that documentation on transient props. I didn't know that's how they worked.

We do have some other examples in the codebase like

const StyledButton = styled.button<{ size: Size }>`
which aren't generating this error, but my guess is either because size is lowercase, or because size is an allowed DOM attribute, however it's not an attribute that does anything on a button. I found and inspected this element to see if it would have a size="x-small" just sitting there in the DOM, but surprisingly it didn't. 😕

We could look at also addressing those cases if that's helpful, or just leave them.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

We could look at also addressing those cases if that's helpful, or just leave them.

I'll merge this one now since it's making a warning pop up. I'll open an issue to look at the others and see if we do or don't want to do anything there.

@robertbrignull robertbrignull merged commit ec0e74b into main Oct 18, 2023
@robertbrignull robertbrignull deleted the robertbrignull/columnCount branch October 18, 2023 17:29
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.

2 participants