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

fix(catalogue): Year 0 harmonisation matrix check mark is displayed while there is no data for this mapping. #4550

Merged
merged 3 commits into from
Dec 15, 2024

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Dec 7, 2024

closes #4428

@mswertz mswertz self-assigned this Dec 7, 2024
@mswertz mswertz requested review from hslh and BrendaHijmans December 7, 2024 21:32
hslh
hslh previously approved these changes Dec 10, 2024
Copy link
Contributor

@hslh hslh left a comment

Choose a reason for hiding this comment

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

Functionality on preview is correct, fixes the original issue.

However... now I've noticed that the text under DEFINITION still says 'Repeated for Year 0-10'. Is that something to fix in this PR as well, or should I make a separate issue?
Screenshot 2024-12-10 at 11 32 48

@mswertz
Copy link
Member Author

mswertz commented Dec 10, 2024

Functionality on preview is correct, fixes the original issue.

However... now I've noticed that the text under DEFINITION still says 'Repeated for Year 0-10'. Is that something to fix in this PR as well?

will take a look. might be same issue with some wrong defaults.

@mswertz
Copy link
Member Author

mswertz commented Dec 13, 2024

I checked but the demo data is 0-10. So actually my bug fix is incorrect (like Joris said)

@mswertz mswertz requested a review from hslh December 13, 2024 13:31
@mswertz mswertz dismissed hslh’s stale review December 13, 2024 13:31

fix was actuallly wrong. can you retry?

@hslh
Copy link
Contributor

hslh commented Dec 13, 2024

I see, my comment was wrong. The variable has repeats 0-10, but all the variables from the testCohorts only have mappings 1-10. So the text under DEFINITION was correct.

Copy link
Contributor

@hslh hslh left a comment

Choose a reason for hiding this comment

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

Looks good to me

@mswertz mswertz merged commit 0045eec into master Dec 15, 2024
7 checks passed
@mswertz mswertz deleted the fix/4428 branch December 15, 2024 11:12
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.

Year 0 harmonisation details check mark is displayed while there is no data for this mapping
2 participants