Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-34366: Migrate 'components/post_view/post_list_virtualized' and associated tests to TypeScript #8516

Closed
wants to merge 48 commits into from

Conversation

naftalimurgor
Copy link

Summary

This Pull Request Migrates components/post_view/post_list_virtualized and associated tests to TypeScript as
described in this issue mattermost/mattermost#17289

Ticket Link

JIRA: https://mattermost.atlassian.net/browse/MM-34366
issue: mattermost/mattermost#17289

Related Pull Requests

  • Has no server changes
  • Has no mobile changes

Screenshots

Release Note

NONE

…cript

add type definitions for 'dynamic-virtualized-list` package.
…cript

Refactor tests in `post_list_virtualized.test.jsx` for PostList component.
…cript

lint `dynamic-virtualized-list.d.ts` to remove trailing white spaces.
Make `isRhsPost` prop optional in `components/post_view/floating_timestamp/floating_timestamp.tsx` as it is not explicitly required in `/home/slax/git/enterprise/mattermost-webapp/components/post_view/post_list_row/post_list_row.tsx`
Update `togglePostMenu` function prop to accept `opened` as a boolean argument
change `post` and `channel` props to optional as they are not explicitly required in `PostListRow` component
…st_virtualized.tsx` to TypeScript

Add type definitions and all relevant type definitions imports.
Migrate 'components/post_view/post_list_virtualized' and associated tests to TypeScript
add `@types/react-virtualized-auto-sizer` from npm
`lastViewedAt` prop is an optional prop as it is not explicitly required in the unit tests under `post_list_virtualized.test.tsx`
Lint and fix to conform to EsLint rules
@naftalimurgor
Copy link
Author

naftalimurgor commented Aug 3, 2021

@devinbinnie
I resolved the conflict, I think there are some failing checks.

@devinbinnie
Copy link
Member

@naftalimurgor Yep, this is the error I'm seeing:

components/global/product_switcher.tsx:105:17 - error TS2322: Type '"view-grid-outline"' is not assignable to type 'TIconGlyph'.

105                 icon={'view-grid-outline'}
                    ~~~~

  node_modules/@mattermost/compass-components/components/icon-button/IconButton.props.d.ts:8:5
    8     icon: TIconGlyph;
          ~~~~
    The expected type comes from property 'icon' which is declared here on type 'IntrinsicAttributes & PIconButton & RefAttributes<HTMLButtonElement>'

Can we fix this?

@naftalimurgor
Copy link
Author

Ok, I'm working on it 👍. I think the error occured after resolving the conflicts.

fixes this TS error `components/global/product_switcher.tsx:105:17 - error TS2322: Type '"view-grid-outline"' is not assignable to type 'TIconGlyph'.

105                 icon={'view-grid-outline'}
                    ~~~~

  node_modules/@mattermost/compass-components/components/icon-button/IconButton.props.d.ts:8:5
    8     icon: TIconGlyph;
          ~~~~
    The expected type comes from property 'icon' which is declared here on type 'IntrinsicAttributes & PIconButton & RefAttributes<HTMLButtonElement>'`
Merge into `mattermost-master` to synchronize changes.
@naftalimurgor
Copy link
Author

naftalimurgor commented Aug 6, 2021

@naftalimurgor Yep, this is the error I'm seeing:

components/global/product_switcher.tsx:105:17 - error TS2322: Type '"view-grid-outline"' is not assignable to type 'TIconGlyph'.

105                 icon={'view-grid-outline'}
                    ~~~~

  node_modules/@mattermost/compass-components/components/icon-button/IconButton.props.d.ts:8:5
    8     icon: TIconGlyph;
          ~~~~
    The expected type comes from property 'icon' which is declared here on type 'IntrinsicAttributes & PIconButton & RefAttributes<HTMLButtonElement>'

Can we fix this?

Hello 👋🏽 @devinbinnie
I looked into the error that was causing
the failed checked, though a bit later and saw that it has been fixed so I resolved some merge conflicts to incorporate the changes.

@naftalimurgor
Copy link
Author

@devinbinnie
Hello, The failing check has already been fixed.

@devinbinnie
Copy link
Member

Thanks @naftalimurgor, looks like there are a couple conflicts though. Can we fix those up? :)

'mattermost-master' to resolve conflicts and update upstream repo.
@devinbinnie
Copy link
Member

Hey @naftalimurgor, thanks for merging master, but it looks like there's a test failing now. Can you have a look at that?

@naftalimurgor
Copy link
Author

naftalimurgor commented Aug 10, 2021

Ok. I ran all the tests and several others were failing too.

@naftalimurgor
Copy link
Author

@devinbinnie
Could this be because of the snapshots?

@devinbinnie
Copy link
Member

@devinbinnie
Could this be because of the snapshots?

Yeah it's possible, you can update them by running npm test -- -u

@naftalimurgor
Copy link
Author

@devinbinnie
I did that, some snapshots got updated, is this ok? :)

@devinbinnie
Copy link
Member

@devinbinnie
I did that, some snapshots got updated, is this ok? :)

Looks like it's still failing, here's the error its spitting out:

Summary of all failing tests
 FAIL  components/post_view/post_list/post_list.test.jsx
  ● components/post_view/post_list › snapshot for loading when there are no posts

    Unexpected console logsWarning: Failed prop type: The prop `atOldestPost` is marked as required in `PostList`, but its value is `undefined`.
        in PostList,Warning: Failed prop type: The prop `latestPostTimeStamp` is marked as required in `PostList`, but its value is `undefined`.
        in PostList

      89 |     if (warns.length > 0 || errors.length > 0) {
      90 |         const message = 'Unexpected console logs' + warns + errors;
    > 91 |         throw new Error(message);
         |               ^
      92 |     }
      93 | });
      94 |

      at Object.<anonymous> (tests/setup.js:91:15)

You can get this output yourself as well by running npm run test

@naftalimurgor
Copy link
Author

I ran npm test -- -u locally and some snapshots got updated. Is this ok?

@naftalimurgor
Copy link
Author

I think it's the recent merge conflict, I'll have to look at how I resolved them. I think it's the reason why tests are failing.

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@devinbinnie
Copy link
Member

@naftalimurgor How's things going? Let us know if you need any help with this.

@naftalimurgor
Copy link
Author

@naftalimurgor How's things going? Let us know if you need any help with this.

Hey @devinbinnie I think I'll have to re-clone this repository to do away with the breaking stuff and resubmit the PR ensuring all the checks pass

@jasonblais
Copy link
Contributor

@naftalimurgor let us know if we can help with the resubmission. Really appreciate your help with this contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Lifecycle/1:stale release-note-none
Projects
None yet
5 participants