Skip to content
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

[BI-1056] All Germplasm Table #165

Merged
merged 12 commits into from
Jan 12, 2022
Merged

[BI-1056] All Germplasm Table #165

merged 12 commits into from
Jan 12, 2022

Conversation

ctucker3
Copy link
Contributor

@ctucker3 ctucker3 commented Jan 5, 2022

Description

Story: (https://breedinginsight.atlassian.net/jira/software/c/projects/BI/boards/1?modal=detail&selectedIssue=BI-1056)

Includes:

Dependencies

sgn: BI-1056 (Breeding-Insight/sgn#50)
bi-api: BI-1056 (Breeding-Insight/bi-api#130)
bi-web: BI-1056 (#165)

Testing

Use the importer and the file below to import some germplasm. Play around with the table and try some paging out to make sure everything works.

BI-1056_lots_of_germplasm.xls

TAF Results

The taf job seems to be broken and is not printing out results.

Checklist:

  • I have performed a self-review of my own code
  • I have tested my code and ensured it meets the acceptance criteria of the story
  • I have create/modified unit tests to cover this change
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to documentation
  • I have run TAF.

@ctucker3 ctucker3 marked this pull request as ready for review January 5, 2022 19:12
@ctucker3 ctucker3 requested review from a team, timparsons and davedrp and removed request for a team January 5, 2022 19:12
Copy link
Member

@timparsons timparsons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BrAPI models that were added are in the directory "brapps/reporter/model". It might be better to move them to "brapi/model".

Comment on lines 20 to 21
import {ResultGenerator} from "@/breeding-insight/model/Result";
import {Germplasm} from "@/breeding-insight/brapps/reporter/model/germplasm";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These imports aren't used and could be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

tag="li"
active-class="is-active"
>
<a>Germplasm</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the story, this text should be "All Germplasm"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

path: 'germplasm-all',
name: 'germplasm-all',
meta: {
title: 'Germplasm',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the techspecs, this text should be "All Germplasm"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

<template>
<section id="germplasmTable">
<h1 class="title">
Germplasm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be "All Germplasm" to match the nav

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

@ctucker3 ctucker3 requested a review from timparsons January 10, 2022 14:42
@ctucker3 ctucker3 merged commit 1e33723 into develop Jan 12, 2022
@ctucker3 ctucker3 deleted the feature/BI-1056 branch January 12, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants