-
Notifications
You must be signed in to change notification settings - Fork 21
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
[AN-149] Rename 'namespace' to 'collection' & update feature preview section #5212
Changes from 4 commits
99683ba
069a58d
2bedd00
fb78de1
3a6cd53
fd7ee7f
069f3cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,12 +97,13 @@ const featurePreviewsConfig: readonly FeaturePreview[] = [ | |
}, | ||
{ | ||
id: FIRECLOUD_UI_MIGRATION, | ||
title: 'Firecloud UI Feature Migration', | ||
description: 'Enabling this feature will update replaceable links to Firecloud UI with new links to Terra UI', | ||
feedbackUrl: `mailto:[email protected]?subject=${encodeURIComponent( | ||
'Feedback on deprecating Firecloud UI' | ||
title: 'Terra Workflow Repository Improvements', | ||
description: | ||
"Enabling this feature will gives users access to the new Terra Workflow Repository UI. This replaces the Broad Methods Repository UI (and doesn't affect any workflows previously created).", | ||
feedbackUrl: `mailto:[email protected]?subject=${encodeURIComponent( | ||
'Feedback on Terra Workflow Repository Improvements' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: This was requested by Beth and I did it as part of this PR itself. Please let me know if there is any wording improvements that can be done, especially for the description. |
||
)}`, | ||
lastUpdated: '3/22/2024', | ||
lastUpdated: '1/7/2025', | ||
articleUrl: 'https://support.terra.bio/hc/en-us/articles/31191238873243', | ||
}, | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,7 +291,7 @@ export const WorkflowsContainer = (props: WorkflowContainerProps) => { | |
}), | ||
permissionsModalOpen && | ||
h(PermissionsModal, { | ||
versionOrNamespace: 'Version', | ||
versionOrCollection: 'Version', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incredibly glad we avoided working on snapshot => version and namespace => collection at the exact same time. |
||
namespace, | ||
setPermissionsModalOpen, | ||
refresh: loadSnapshot, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,7 @@ validate.validators.maxNamespaceNameCombinedLength = <OtherFieldName extends str | |
attributes: Record<OtherFieldName, string> | ||
): string | null => | ||
value.length + attributes[options.otherField].length > 250 | ||
? '^Namespace and name are too long (maximum is 250 characters total)' // ^ character prevents attribute from being prepended | ||
? '^Collection and workflow names are too long (maximum is 250 characters total)' // ^ character prevents attribute from being prepended | ||
: null; | ||
|
||
const createWorkflowModalConstraints = { | ||
|
@@ -77,15 +77,15 @@ const createWorkflowModalConstraints = { | |
}, | ||
}; | ||
|
||
interface NamespaceNameSectionProps { | ||
interface CollectionAndNameSectionProps { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to also modify namespace to collection inside the interface as well? Also what does name refers to here, the collection name or the workflow name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had intentionally decided to rename only user facing and component names to Having said that, I do understand that there might be some confusion when someone else comes and looks at the code. So I am open to the discussion on renaming what else what be good for developer readability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, in that case maybe a little comment in the code explaining that a namespace is the collection, and name is the workflow name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I can do that. I will also update the component name to be |
||
namespace: string | undefined; | ||
name: string | undefined; | ||
setWorkflowNamespace: (value: string) => void; | ||
setWorkflowName: (value: string) => void; | ||
errors: any; | ||
} | ||
|
||
const NamespaceNameSection = (props: NamespaceNameSectionProps) => { | ||
const CollectionAndNameSection = (props: CollectionAndNameSectionProps) => { | ||
const { namespace, name, setWorkflowNamespace, setWorkflowName, errors } = props; | ||
const [namespaceModified, setNamespaceModified] = useState<boolean>(false); | ||
const [nameModified, setNameModified] = useState<boolean>(false); | ||
|
@@ -98,7 +98,7 @@ const NamespaceNameSection = (props: NamespaceNameSectionProps) => { | |
<div style={{ flexWrap: 'wrap', flexGrow: 1, flexBasis: '400px' }}> | ||
<div style={{ marginBottom: '0.1667em' }}> | ||
<FormLabel htmlFor={namespaceInputId} required> | ||
Namespace | ||
Collection name | ||
</FormLabel> | ||
<ValidatedInput | ||
inputProps={{ | ||
|
@@ -117,7 +117,7 @@ const NamespaceNameSection = (props: NamespaceNameSectionProps) => { | |
<div style={{ flexWrap: 'wrap', flexGrow: 1, flexBasis: '400px' }}> | ||
<div style={{ marginBottom: '0.1667em' }}> | ||
<FormLabel htmlFor={nameInputId} required> | ||
Name | ||
Workflow name | ||
</FormLabel> | ||
<ValidatedInput | ||
inputProps={{ | ||
|
@@ -165,7 +165,8 @@ export const CreateWorkflowModal = (props: CreateWorkflowModalProps) => { | |
|
||
const validationErrors = validate({ namespace, name, synopsis, wdl }, createWorkflowModalConstraints, { | ||
prettify: (v) => | ||
({ namespace: 'Namespace', name: 'Name', synopsis: 'Synopsis', wdl: 'WDL' }[v] || validate.prettify(v)), | ||
({ namespace: 'Collection name', name: 'Workflow name', synopsis: 'Synopsis', wdl: 'WDL' }[v] || | ||
validate.prettify(v)), | ||
}); | ||
|
||
const onSubmitWorkflow = withBusyState(setBusy, async () => { | ||
|
@@ -190,7 +191,7 @@ export const CreateWorkflowModal = (props: CreateWorkflowModalProps) => { | |
> | ||
<div style={{ padding: '0.5rem 0' }}> | ||
<div style={{ display: 'flex', alignItems: 'flex-end', flexWrap: 'wrap', gap: '16px' }}> | ||
<NamespaceNameSection | ||
<CollectionAndNameSection | ||
namespace={namespace} | ||
name={name} | ||
setWorkflowNamespace={setNamespace} | ||
|
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 attempted to emphasize what had been missing in Terra (create & edit) and try to explain that these are two UIs viewing the same backend data.
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.
What do you think about this? This also conveys that Broad Methods Repository is being replaced. 🤔
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.
Seems good to me!