feat(templates): Support auto selecting template #6485
Conversation
tonytang-microsoft-com
requested review from
Elaina-Lee,
Copilot and
preetriti1
| export interface TemplatesDesignerProps { | ||
| detailFilters: TemplateDetailFilterType; | ||
| createWorkflowCall: CreateWorkflowHandler; | ||
| viewTemplate?: { | ||
| templateName?: string; |
There was a problem hiding this comment.
Could we make this an non-optional field? If optional param of viewTemplate is present, we should assume templateName is present.
| useEffect(() => setLayerHostSelector('#msla-layer-host'), []); | ||
| const intl = useIntl(); | ||
| const dispatch = useDispatch<AppDispatch>(); | ||
|
|
||
| const [locked, setLocked] = useState(false); |
There was a problem hiding this comment.
This locked state is only being used in useEffect to check if it has been set to true. I'm wondering since it's in useEffect, if it's only being used to ensure this is run once. If so, since we are already keeping templateLocked in the state, we should use this for the check.
| filteredTemplateNames, | ||
| filters: { pageNum, detailFilters: appliedDetailFilters }, | ||
| } = useSelector((state: RootState) => state.manifest); | ||
| const selectedTabId = appliedDetailFilters?.Type?.[0]?.value; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
These logic should reside in TemplatesDataProvider to keep related functionality centralized. Since this modifies the state determining which templates are displayed, I would suggest it makes the most to keep similar logic together for better organization and coherence.
| } | ||
| } | ||
| } | ||
| }, [dispatch, availableTemplates, viewTemplate, filteredTemplateNames]); |
There was a problem hiding this comment.
In case where we don't list all the variables used within useEffect in the dependency list intentionally, we put // eslint-disable-next-line react-hooks/exhaustive-deps on the top of the dependency list
| @@ -5,6 +5,7 @@ import { getCurrentWorkflowNames, validateConnectionsValue, validateParameterVal | |||
| import { initializeTemplateServices, loadTemplate, validateWorkflowName, type TemplatePayload } from '../../actions/bjsworkflow/templates'; | |||
|
|
|||
| export interface TemplateState extends TemplatePayload { | |||
| templateNameLocked?: boolean; | |||
There was a problem hiding this comment.
I would like to suggest to renaming this to isTemplateNameLocked
| @@ -69,6 +71,10 @@ export const CreateWorkflowPanel = ({ createWorkflow, onClose, clearDetailsOnClo | |||
| }; | |||
|
|
|||
| const dismissPanel = useCallback(() => { | |||
| if (templateNameLocked) { | |||
There was a problem hiding this comment.
This value is not added to the dependency list of useCallback
| @@ -177,6 +180,7 @@ export const useCreateWorkflowPanelTabs = ({ | |||
| nextTabId: parametersExist ? Constants.TEMPLATE_PANEL_TAB_NAMES.PARAMETERS : Constants.TEMPLATE_PANEL_TAB_NAMES.REVIEW_AND_CREATE, | |||
| hasError: !!connectionsError, | |||
| isCreating, | |||
| templateNameLocked, | |||
There was a problem hiding this comment.
templateNameLocked is not added in any of the useMemo dependency list for tab items
| if (templateManifest && !locked) { | ||
| setLocked(true); | ||
| dispatch(lockTemplate(viewTemplate.templateName)); | ||
| dispatch(loadTemplate({ preLoadedManifest: templateManifest })); |
There was a problem hiding this comment.
we need to pass isCustomTemplate boolean here, otherwise this will default to assuming it is present in github. Since we are checking over filteredTemplateNames, this could include custom templates.
|
aborting the PR since Elaina has forked the branch and made more changes. |
Type of Change
New Behavior
Add support to auto selecting template. When this new parameter is set, the template designer will automatically select the template as if user clicked the template and create.
Impact of Change
This is additive change so it's fully back compatible
Test Plan
Tested locally and added CIT
Screenshots or Videos (if applicable)