-
Notifications
You must be signed in to change notification settings - Fork 3
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
[MI-2061]: Refactoring and optimisations around Forms, RTK query, and types and constants management #45
Conversation
…re-devops into MI-1987_1
…n with other changes
…and a centralised check for root modals
type Props = { | ||
fieldConfig: Pick<ModalFormFieldConfig, 'label' | 'type' | 'validations'> | ||
value: any | ||
optionsList: any |
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.
OptionsList will always be LabelValuePair[]
, 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.
yes, this has been changed in later PR not fixing here to avoid conflicts
} = useForm(plugin_constants.form.subscriptionModal); | ||
const {getApiState, makeApiRequest, makeApiRequestWithCompletionStatus, state} = usePluginApi(); | ||
const {visibility} = getSubscribeModalState(state); | ||
const {entities} = useSelector((reduxState: GlobalState) => reduxState); |
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.
const {entities} = useSelector((reduxState: GlobalState) => reduxState); | |
const {currentTeamId} = useSelector((reduxState: GlobalState) => reduxState.entities.teams); |
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 change is done by @ayusht2810 but I've fixed it here
} | ||
|
||
const projectList = getProjectState().data; | ||
if (projectList) { | ||
setProjectOptions(getProjectList(projectList)); | ||
setOrganizationOptions(getOrganizationList(projectList)); | ||
} | ||
}, [usePlugin.state]); | ||
}, [state]); |
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.
Use specific values here instead of using the whole state.
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 change is done by @ayusht2810 will be modified by him in his PR
useEffect(() => { | ||
const channelList = getChannelState().data; | ||
if (channelList) { | ||
setChannelOptions(channelList.map((channel) => ({label: <span><i className='fa fa-globe dropdown-option-icon'/>{channel.display_name}</span>, value: channel.id}))); | ||
setChannelOptions(channelList.map((channel) => ({ | ||
label: <span><i className='fa fa-globe dropdown-option-icon'/>{channel.display_name}</span>, |
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.
Add condition for public and private channels and use the same icons used by MM.
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 change is done by @ayusht2810
also same has been fixed in later PR
webapp/src/selectors/index.tsx
Outdated
}; | ||
|
||
export const getApiQueriesState = (state: any): ApiQueriesState => { | ||
return state[pluginPrefix].azureDevopsPluginApi?.queries; |
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.
return state[pluginPrefix].azureDevopsPluginApi?.queries; | |
return state[pluginPrefix].azureDevopsPluginApi.queries; |
Is there really a need for optional chaining here?
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.
yes this is async so it may be undefined initially
…re-devops into MI-2061
Changes are: