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

feat: aggregation dashboard #3705

Merged
merged 11 commits into from
May 22, 2024
Merged

feat: aggregation dashboard #3705

merged 11 commits into from
May 22, 2024

Conversation

davidruvolo51
Copy link
Contributor

@davidruvolo51 davidruvolo51 commented May 4, 2024

Due to merge conflicts and rebase issues with #3376, a new branch was created and this PR was opened.

What are the main changes you did:

  • Addition of aggregation dashboard that works with the table Catalogue/Aggregation.
  • Added table for metastasis
  • Added filter for resources
  • Added page header

how to test:

  • this UI is accessible in any database that uses the catalogue schema: url = catalogue-demo/aggregation-dashboard

Input needed / notes:

  • Is it better to create a generic page or would you like it to be branded for a project? Is there a better name for the page?
  • Please review chart titles. Do they make sense? Is there a better way to phrase the charts?
  • Sample type is no longer part of the model, right? (I've removed this since it wasn't in the new datamodel)
  • Should I add a section on "how to use the dashboard"?
  • I'm not using the MolgenisVizEmx components. I still need to expand the library and enable filters. For now, I will continue with the current app structure.

Todo:

  • adjusted left margin on research center chart
  • when a row in primary tumor site table is clicked, the filters aren't generated
  • Update chart titles
  • fix bug where filters generated by cases by primary tumor chart disappear
  • remove on click event that updates charts when filter are applied
  • fix sorting of pie chart data
  • fix gap around app: needs a new PR

@davidruvolo51 davidruvolo51 self-assigned this May 4, 2024
@davidruvolo51 davidruvolo51 mentioned this pull request May 4, 2024
19 tasks
@davidruvolo51
Copy link
Contributor Author

For a new PR(s) -

  • A recent PR introduced a new main element. Remove the one that was generated by the molgenis-viz library
  • On the new main element, remove the class p-3 and figure out what is causing the extra white space before the app

@davidruvolo51 davidruvolo51 marked this pull request as ready for review May 5, 2024 13:08
@davidruvolo51
Copy link
Contributor Author

The margins and chart label work break is now working. However, some of the labels have a forward slash. Do you want to use a regex here to break labels at spaces and slashes? Or should these values be revised at the source?

Copy link
Contributor

@BrendaHijmans BrendaHijmans left a comment

Choose a reason for hiding this comment

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

This looks great, David! I have some minor comments:
-Is the 'apply filters' button necessary? I found it a bit hard to notice that the filters were not applied straight away by clicking on the graphs.
-The labels of the x-axis of the sampling period overlap, making them illegible.

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

Code looks good. If Brenda says 'go' we can go.

@BrendaHijmans
Copy link
Contributor

The figure "Number of cases by stage at diagnosis" should also specify that this is about distant metastasis. Can you make this: "Number of cases by stage at diagnosis, distant metastasis" yes no unknow

@BrendaHijmans
Copy link
Contributor

PRIMARYTUMORSITE - can this contain spaces between the words?

Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@davidruvolo51 davidruvolo51 merged commit ba30b75 into master May 22, 2024
5 of 6 checks passed
@davidruvolo51 davidruvolo51 deleted the feat/aggregation-dashboard branch May 22, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants