-
Notifications
You must be signed in to change notification settings - Fork 9
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
MM-59373: Respecting name display preference in boards #22
Conversation
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.
LGTM
className='comment-text' | ||
dangerouslySetInnerHTML={{__html: html}} | ||
/> | ||
<Provider store={(window as any).store}> |
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.
Can you share why do we need to create a store provider here? It does't look right.
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.
The Provider is necessary in this context because the function messageHtmlToComponent
depends on certain state and actions that are part of the mmStore which is exposed on window object.
webapp/src/store/teams.ts
Outdated
@@ -87,3 +93,19 @@ export const getCurrentTeamId = (state: RootState): string => state.teams.curren | |||
export const getCurrentTeam = (state: RootState): Team|null => state.teams.current | |||
export const getFirstTeam = (state: RootState): Team|null => state.teams.allTeams[0] | |||
export const getAllTeams = (state: RootState): Team[] => state.teams.allTeams | |||
|
|||
|
|||
export const getChannelsNameMapInTeam: (state: GlobalState, teamId: string) => Record<string, Channel> = createSelector( |
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.
This can be removed as its not being used. We're using the selector directly from MM redux package.
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.
Done.
webapp/src/webapp_globals.ts
Outdated
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
export const { |
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.
You need to defined the types like how its done in user survey plugin for example - https://github.com/mattermost/mattermost-plugin-user-survey/blob/29685cc4d7ca0a2e1de196c23f6165caeacaf1b4/webapp/src/types/mattermost-webapp/index.d.ts#L10
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.
Done.
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.
Looks great! I'm going to take a look at how to better ship the web app's Markdown code for the hackathon later this week, so I'm looking forward to building off this and hopefully making this much easier next time it happens
import Comment from './comment' | ||
|
||
jest.mock('../../mutator') | ||
const mockedMutator = mocked(mutator, true) | ||
jest.mock('../../webapp_globals', () => ({ | ||
...jest.requireActual('../../webapp_globals'), | ||
messageHtmlToComponent: jest.fn(() => <div className="mocked-message-html">Test Comment</div>), |
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.
This isn't part of this PR, but when I saw this, I realized that whenever we get to shipping a plugin API package from the web app, it should include mock versions of all the APIs to make writing tests that use them easier.
In the mean time, you could add this to the Jest setup script to avoid having to redefine it in multiple test files
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.
@hmhealey Looking at the repo now, I realize that jest.config.js
isn’t configured in the board/focalboard repo. I’m planning to set up the proper Jest configuration in a follow-up PR, which will include the changes mentioned above.
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.
It's actually configured as part of the package.json
currently, but moving it out to it's own file and setting it up a bit better makes sense to me. While it's nice to use fewer files sometimes, I prefer having it spread out since the package.json
already does so much stuff
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.
Ah, got it. Done.
@yasserfaraazkhan gentle ping on this one so we can include this last PR in the boards release. |
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.
Summary
Added support to respect name display preference from channel to boards.
Ticket Link
https://mattermost.atlassian.net/browse/MM-59373