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

[DUOS-2845][risk=no] Fix dac datasets modal bug #2422

Merged
merged 3 commits into from
Dec 14, 2023

Conversation

rushtong
Copy link
Contributor

@rushtong rushtong commented Dec 13, 2023

Addresses

https://broadworkbench.atlassian.net/browse/DUOS-2845

Summary

Refactor the dac datasets modal to be a newer functional component and fix the error when using the read more component.

Also note that we're removing Consent ID and Sample Collection ID from the display ... those are no longer supported in the new Dataset/Study model.

Why

Using the ReadMore component as part of the translated data use caused a problem with too many useEffect hooks in combination with the parent components. The "fix" here is to use a shortened (or ellipses concatenated string) along with using the full translation as the title text of the span it lives in and remove the use of the ReadMore component altogether.


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rushtong rushtong requested a review from a team as a code owner December 13, 2023 20:05
Copy link
Contributor

@hams7504 hams7504 left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

Copy link

@Ghost-in-a-Jar Ghost-in-a-Jar left a comment

Choose a reason for hiding this comment

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

I am not really very familiar with the DUOS UI but this seems reasonable.

I think it would maybe be worth converting DacDatasetsModal to ts, the ts-ignore comments peppered around worry me a bit, but I didnt want to block fixing a bug.

@rushtong
Copy link
Contributor Author

think it would maybe be worth converting DacDatasetsModal to ts, the ts-ignore comments peppered around worry me a bit, but I didnt want to block fixing a bug.

Thank you @Ghost-in-a-Jar - yes, I was looking into that too. The TS conversion is non-trivial at the moment since we are just starting that journey. I do intend to do that, but was hoping to get past the bug first.

@rushtong rushtong merged commit 8a14196 into develop Dec 14, 2023
9 checks passed
@rushtong rushtong deleted the gr-DUOS-2845-fix-dac-datasets-modal branch December 14, 2023 15:54
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.

3 participants