-
Notifications
You must be signed in to change notification settings - Fork 31
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
Displaying identifiers for project drop downs #1352
Changes from 7 commits
d40aa12
df7ca7d
d5ee224
5a7c0d2
5cf58ef
24d03cf
e872b31
76ae170
ca4b83f
7f2abe6
7bfbc4e
3aa96c6
76ba866
8023bed
d19ce2b
9942971
0d340a1
5c31979
ddfd9e6
ee728de
4e51db1
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 |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import React from "react"; | ||
import { Select, SelectProps, Tag, Typography } from "antd"; | ||
|
||
export type Project = { identifier: number; name: string }; | ||
joshsadam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export interface ProjectSelectProps extends SelectProps { | ||
projects: Project[]; | ||
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. You need to describe all you parameters 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. Reached out about this comment. Instructed to disregard. 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 though you had decided to go with 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. Please see 76ba866. 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. After generalising the component, I think picking id and name form IridaBase makes the most sense now. Please see 9942971. |
||
} | ||
|
||
/** | ||
* React component for displaying a project drop-down menu. | ||
* @param {list} projects - list of projects that is to be displayed | ||
* @param {function} onChange - function that is called when select option has changed | ||
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. When using TypeScript you do not need to add the type also to the JSDoc. 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. Please see 76ae170. |
||
* @param {number} defaultValue - project identifier of the project that is to be displayed by default | ||
* @returns {JSX.Element} | ||
* @constructor | ||
*/ | ||
export function ProjectSelect({ | ||
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 wonder we could generalize this to be a select for anything that has an id? 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 think so... I just need a good name. Please see d19ce2b. |
||
projects, | ||
onChange = undefined, | ||
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 doubt you want the 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. Please see 8023bed. |
||
defaultValue = null, | ||
}: ProjectSelectProps): JSX.Element { | ||
const [options, setOptions] = React.useState(() => formatOptions(projects)); | ||
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. You need to state what type the options are. I think the best way that I have come across is: const [options, setOptions] = React.useState<SelectOptions[]>(()=>formatOptions(projects)); 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. Please see 7bfbc4e. |
||
|
||
function formatOptions(values: Project[]) { | ||
if (!values) return []; | ||
return values.map((project) => ({ | ||
label: ( | ||
<div | ||
style={{ | ||
display: "flex", | ||
justifyContent: "space-between", | ||
width: "100%", | ||
}} | ||
> | ||
<Typography.Text ellipsis={{ tooltip: true }}> | ||
{project.name} | ||
</Typography.Text> | ||
<Tag>{i18n("ProjectSelect.label.id", project.identifier)}</Tag> | ||
</div> | ||
), | ||
value: project.identifier, | ||
selected: project.name, | ||
})); | ||
} | ||
|
||
React.useEffect(() => { | ||
setOptions(formatOptions(projects)); | ||
}, [projects]); | ||
|
||
const handleSearch = (value: string) => { | ||
const lowerValue = value.toLowerCase(); | ||
|
||
const available = projects.filter( | ||
(project) => | ||
project.name.toLowerCase().includes(lowerValue) || | ||
project.identifier.toString() === value | ||
); | ||
const formatted = formatOptions(available); | ||
setOptions(formatted); | ||
}; | ||
|
||
return ( | ||
<Select | ||
optionLabelProp="selected" | ||
autoFocus | ||
showSearch | ||
size="large" | ||
style={{ width: `100%` }} | ||
options={options} | ||
className="t-project-select" | ||
filterOption={false} | ||
onSearch={handleSearch} | ||
onChange={onChange} | ||
defaultValue={defaultValue} | ||
/> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
import static ca.corefacility.bioinformatics.irida.ria.integration.pages.AbstractPage.waitForTime; | ||
|
||
public class ShareSamplesPage { | ||
@FindBy(css = ".t-share-project .ant-select-selection-search-input") | ||
@FindBy(css = ".t-project-select .ant-select-selection-search-input") | ||
private WebElement shareProjectSelect; | ||
|
||
@FindBy(className = "ant-select-dropdown") | ||
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. Can you add testing for searching for a project by it's identifier please 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. Please see 3aa96c6. |
||
|
@@ -147,15 +147,15 @@ public boolean isShareSingleSuccessDisplayed() { | |
public boolean isSomeSamplesSameIdsWarningDisplayed() { | ||
try { | ||
return someSamplesSameIdsWarning.isDisplayed(); | ||
} catch(Exception e) { | ||
} catch (Exception e) { | ||
return false; | ||
} | ||
} | ||
|
||
public boolean isSomeSamplesSameNamesWarningDisplayed() { | ||
try { | ||
return someSamplesSameNamesWarning.isDisplayed(); | ||
} catch(Exception e) { | ||
} catch (Exception e) { | ||
return false; | ||
} | ||
} | ||
|
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 try using uppercase on the ID, I think it might look better.
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.
Please see ca4b83f.