-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
refactor(typescript): Convert TableHead, TableHeader to tsx #13367
refactor(typescript): Convert TableHead, TableHeader to tsx #13367
Conversation
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Thanks for putting this together @pratikkarad! Just one change request regarding the thead
interface. also looks like some of the CI checks are failing
@@ -91,7 +158,7 @@ const TableHeader = React.forwardRef(function TableHeader( | |||
[`${prefix}--table-sort--descending`]: | |||
isSortHeader && sortDirection === sortStates.DESC, | |||
}); | |||
const ariaSort = !isSortHeader ? 'none' : sortDirections[sortDirection]; | |||
const ariaSort = !isSortHeader ? 'none' : sortDirection ? sortDirections[sortDirection] : sortDirections[sortStates.NONE]; |
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.
I observed It is still failing here. I tried debugging but got no luck. Any help or suggestion is appreciated. 😃
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.
let's change this line to const ariaSort = (!isSortHeader || !sortDirection) ? 'none' : sortDirections[sortDirection];
and change the type of sortDirections
in the top to
const sortDirections: {[key: string]: 'none' | 'ascending' | 'descending'} = {
[sortStates.NONE]: 'none',
[sortStates.ASC]: 'ascending',
[sortStates.DESC]: 'descending',
};
That should fix this issue
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.
Hi @francinelucca, I'm facing a problem with declaring the static properties on the functional component in typescript.
For eg.
TableHeader.translationKeys = Object.values(translationKeys);
In TableHeader, translationKeys
is giving the compilation error.
I tried creating a separate type as suggested on StackOverflow/blogs, but still got no luck.
can you try doing |
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Closes #12527, #12528
Converts TableHead and TableHeader to tsx in preparation for the main Table conversion
Changelog
Changed
Testing / Reviewing
To verify these changes are working, we can check to see if the existing storybook behavior is still working correctly. Should be able to properly import converted components in a TS environment