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

⌨️ #1098 Add Data Center fields #2754

Merged
merged 12 commits into from
Feb 21, 2024
Merged

Conversation

demariadaniel
Copy link
Contributor

@demariadaniel demariadaniel commented Feb 15, 2024

Description of changes

Adds DataCenter fields to UI pages & Queries (especially Create / Update Program)

Related API Changes: icgc-argo/platform-api#717

Screenshots: https://app.zenhub.com/workspaces/icgc-argo-platform-dk-production-board-5e542d38415f5034e9fed89d/issues/gh/icgc-argo/roadmap/1098

Type of Change

  • Bug
  • Dependency updates
  • Feature
  • Refactoring
  • Release candidate
  • Styling
  • Testing
  • Other (please describe)

Checklist before requesting review

  • Design (select one):
    • Matches design:
      • component sizes, spacing, and styles
      • font size, weight, colour
      • spelling has been double checked
    • No design provided, screenshot of changes included in PR
    • No changes to UI
  • Back-end (select one):
    • Back-end PRs merged, tested on develop, and linked in this PR
    • No back-end changes
  • Manually tested changes
  • Added copyrights to new files
  • Connected ticket to PR

@demariadaniel demariadaniel changed the title WIP: #1098 Add Data Center fields 💾 #1098 Add Data Center fields Feb 20, 2024
@demariadaniel demariadaniel changed the title 💾 #1098 Add Data Center fields ⌨️ #1098 Add Data Center fields Feb 20, 2024
* if using a shortName format that is not "NAME-AREA", a working example is "BOB-CA"
* using cancerTypes or countries that doesn't exist or is not in the same format as the database ex. a working example of cancerTypes is "Lung cancer". Bad example are "lung", "lung cancer"
* admin's email must be in an email format ex. use @
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

If you added this comment to the file directly , it will be deleted when types re-generate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment lives in the API and Daniel wrote it while he was here
I did not add it myself, gql_types has already been generated by running gql:codegen

Copy link
Contributor

Choose a reason for hiding this comment

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

excellent! I didn't know it kept comments, that's great to know.

submissionSongCode
submissionSongUrl
submissionScoreUrl
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels off. Why are we sending all these urls to the frontend when our architecture is using gateways?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, I agree however this is just the UI requesting all Data Center related fields
The URLs will have to be removed upstream @ the API GQL type and I will have to ask if this can be changed
Worth investigating before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of these fields are required so they can be removed from FE query, just going to confirm their intent on Slack

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 based on Slack discussion, not needed for UI

submissionSongCode
submissionSongUrl
submissionScoreUrl
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these can be removed too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry, removed

@ciaranschutte ciaranschutte self-requested a review February 21, 2024 15:58
@demariadaniel demariadaniel merged commit 2a770a3 into develop Feb 21, 2024
2 checks passed
@demariadaniel demariadaniel deleted the feat/1098-dataCenter-fields branch February 21, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants