Displaying identifiers for project drop downs #1352

ksierks · 2022-08-09T15:24:56Z

Description of changes

What did you change in this pull request? Provide a description of files changed, user interactions changed, etc. Include how to test your changes.

Related issue

#1347

Checklist

Things for the developer to confirm they've done before the PR should be accepted:

CHANGELOG.md (and UPGRADING.md if necessary) updated with information for new change.
~~[ ] Tests added (or description of how to test) for any new features.~~
~~[ ] User documentation updated for UI or technical changes.~~

joshsadam · 2022-08-10T14:25:10Z

The layout is broken when you select a project:

joshsadam · 2022-08-10T14:28:01Z

Cannot search by project id:

joshsadam · 2022-08-10T14:36:13Z

You should make this into a reusable component so that it can be used in other places. I cannot think of any other ones that need to be replaced right now, but please see if there are any others.

joshsadam · 2022-08-10T14:42:56Z

src/main/webapp/resources/js/pages/projects/share/ShareProject.jsx

+    <Select.Option key={project.identifier} value={project.identifier}>
+      <>
+        <Text>{project.name}</Text>
+        <Tag style={{ float: "right" }}>{project.identifier}</Tag>


Please do not use float, this is what is causeing the break in your UI when the project is selected.

Please see 5cf58ef.

joshsadam · 2022-08-10T14:43:59Z

src/main/webapp/resources/js/pages/projects/share/ShareProject.jsx

        className="t-share-project"
        filterOption={false}
        onSearch={handleSearch}
        onChange={onChange}
        defaultValue={targetProject ? targetProject.identifier : null}
-      />
+      >
+        {options}


Why did you get rid of options attribute on the select and add it here? This is less performant according to their documentation.

Please see 5cf58ef.

joshsadam · 2022-08-10T14:44:29Z

src/main/webapp/resources/js/pages/projects/share/ShareProject.jsx


  const handleSearch = (value) => {
    const lowerValue = value.toLowerCase();
-    const available = projects.filter((project) =>
+    const filteredProjects = projects.filter((project) =>


You should include searching the project id as well.

Please see 5cf58ef.

joshsadam · 2022-08-10T14:49:54Z

src/main/webapp/resources/js/pages/projects/share/ShareProject.jsx

+    <Select.Option key={project.identifier} value={project.identifier}>
+      <>
+        <Text>{project.name}</Text>
+        <Tag style={{ float: "right" }}>{project.identifier}</Tag>


Can you prepend the id with: id: do the user know what that value is

Please see 5cf58ef.

joshsadam · 2022-08-10T15:13:03Z

3 more things:

Check what happens with long names. You will need to truncate the project name with an ellipsis, and show full with a tooltip or something on hover [This is a really long name that... (id: 2005)].
Try updating this to use TypeScript
TESTING

joshsadam

A couple more quick changes, sorry I will take a longer look at it in the morning. I also have not tried running it yet.

joshsadam · 2022-08-12T02:24:11Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+/**
+ * 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


When using TypeScript you do not need to add the type also to the JSDoc.

Please see 76ae170.

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

joshsadam · 2022-08-12T02:28:45Z

src/main/resources/i18n/messages.properties

@@ -692,6 +692,9 @@ project.nav.details=Recent Activity
 project.nav.settings=Settings
 project.nav.samples.import-metadata=Import Sample Metadata

+# Project select
+ProjectSelect.label.id=id: {0}


Can you try using uppercase on the ID, I think it might look better.

Please see ca4b83f.

joshsadam · 2022-08-12T02:29:54Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+ */
+export function ProjectSelect({
+  projects,
+  onChange = undefined,


I doubt you want the onChange to ever be undefined

Please see 8023bed.

joshsadam · 2022-08-12T02:33:44Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+ * @returns {JSX.Element}
+ * @constructor
+ */
+export function ProjectSelect({


I wonder we could generalize this to be a select for anything that has an id?

I think so... I just need a good name. Please see d19ce2b.

joshsadam · 2022-08-12T02:37:03Z

...va/ca/corefacility/bioinformatics/irida/ria/integration/pages/projects/ShareSamplesPage.java

@@ -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")


Can you add testing for searching for a project by it's identifier please

Please see 3aa96c6.

joshsadam · 2022-08-12T02:39:25Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+  onChange = undefined,
+  defaultValue = null,
+}: ProjectSelectProps): JSX.Element {
+  const [options, setOptions] = React.useState(() => formatOptions(projects));


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));

Please see 7bfbc4e.

joshsadam · 2022-08-16T10:18:50Z

This is coming along well, thanks for the updates. Can you make sure that the project id is displayed after the project is selected, right now just the project name is displayed.

joshsadam

Just a couple more things. also please improve the testing.

joshsadam · 2022-08-16T10:22:44Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+ */
+export function ProjectSelect({
+  projects,
+  onChange = () => {},


I would just not set a default value for the onChange since it really cannot function without one.

Please see 8023bed.

joshsadam · 2022-08-16T10:23:12Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+export type Project = { identifier: number; name: string };
+
+export interface ProjectSelectProps extends SelectProps {
+  projects: Project[];


You need to describe all you parameters

Reached out about this comment. Instructed to disregard.

joshsadam · 2022-08-16T10:23:31Z

src/main/webapp/resources/js/components/project/ProjectSelect.tsx

+export type Project = { identifier: number; name: string };
+
+export interface ProjectSelectProps extends SelectProps {
+  projects: Project[];


I though you had decided to go with ProjectMinimal here?

Please see 76ba866.

After generalising the component, I think picking id and name form IridaBase makes the most sense now. Please see 9942971.

joshsadam · 2022-08-16T10:45:03Z

...ava/ca/corefacility/bioinformatics/irida/ria/integration/projects/ProjectShareSamplesIT.java

@@ -27,6 +27,8 @@ public void testShareSamplesAsManager() {
 		samplesPage.shareSamples();

 		assertFalse(shareSamplesPage.isNextButtonEnabled(), "");
+		shareSamplesPage.searchForProject("2");


This does not really count as searching for a project by it's identifier since the project name also contains a 2. What I would like to see is a completely different name from the identifier and confirm that the id is present in the tag.

Please see 0d340a1.

joshsadam

This looks great. Thanks for all the changes.

ksierks added 4 commits August 5, 2022 15:07

working on showing identifier for share projects drop down

d40aa12

working on showing identifier for share projects drop down

df7ca7d

Merge branch 'development' into display-id-for-project-drop-downs

d5ee224

updating changelog

5a7c0d2

ksierks added the UI User Interface Issue label Aug 9, 2022

ksierks assigned joshsadam Aug 9, 2022

ksierks requested a review from joshsadam August 9, 2022 20:41

joshsadam requested changes Aug 10, 2022

View reviewed changes

ksierks added 3 commits August 10, 2022 15:06

redoing

5cf58ef

making into reusable component

24d03cf

converting to typescript

e872b31

ksierks requested a review from joshsadam August 11, 2022 15:45

joshsadam requested changes Aug 12, 2022

View reviewed changes

ksierks added 5 commits August 12, 2022 14:52

removing types from jsdoc

76ae170

making id uppercase

ca4b83f

fixing default value for onchange

7f2abe6

updating options type

7bfbc4e

testing searching by id

3aa96c6

joshsadam requested changes Aug 16, 2022

View reviewed changes

ksierks added 5 commits August 16, 2022 10:01

using ProjectMinimal type

76ba866

removing onchange default

8023bed

generalizing component

d19ce2b

generalizing component

9942971

fixing test

0d340a1

ksierks requested a review from joshsadam August 16, 2022 19:20

ksierks added 4 commits August 17, 2022 09:23

display id during selection

5c31979

display id during selection

ddfd9e6

Merge branch 'development' into display-id-for-project-drop-downs

ee728de

fixing test

4e51db1

joshsadam approved these changes Aug 23, 2022

View reviewed changes

joshsadam merged commit 318e94a into development Aug 23, 2022

ksierks deleted the display-id-for-project-drop-downs branch October 7, 2022 13:46

ericenns mentioned this pull request Nov 9, 2022

Add project identifier to all drop-downs where the project name is displayed #1347

Closed

ksierks mentioned this pull request Mar 3, 2023

Add project id to project synchronization dropdown #1474

Merged

1 task

Displaying identifiers for project drop downs #1352

Displaying identifiers for project drop downs #1352

Conversation

ksierks commented Aug 9, 2022 • edited Loading

Description of changes

Related issue

Checklist

joshsadam commented Aug 10, 2022

joshsadam commented Aug 10, 2022

joshsadam commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsadam commented Aug 10, 2022 • edited Loading

joshsadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsadam commented Aug 16, 2022

joshsadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsadam left a comment

Choose a reason for hiding this comment

ksierks commented Aug 9, 2022 •

edited

Loading

joshsadam commented Aug 10, 2022 •

edited

Loading