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): variable list should show +n when repeat info is set #4259

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Sep 26, 2024

closes #4259

What are the main changes you did:

  • changes to variables page

how to test:

  • browse to catalogue-demo -> TestNetwork -> Variables -> testRepeatVar_ and see that a +x is shown
  • go to harmonisation matrix, click testRepeatVar_ and see "Repeated for'

todo:

  • fix the +1 bubble
  • fix the popup to also show repeatvar info

@mswertz mswertz marked this pull request as draft September 26, 2024 07:20
@@ -14,6 +14,10 @@ const props = withDefaults(
const variableKey = computed(() => getKey(props.variable));

const resourcePathId = resourceIdPath(variableKey.value);

function repeats(variable: IVariable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

repeats is the max - min ?? , whats use case is this number supporting

Copy link
Member Author

Choose a reason for hiding this comment

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

the number of repeats as before. E.g. when the variable defines are measurements from week 5 - 10 THEN repeats = 5 <= I now see my mistake. Is actually 4?

You might see in popup there is shown as 'week 5 - 10'.
We could replace with this sentence, is just a bit longer but much more useful. What do you think? Otherwise we can consult Brenda or Marije

Copy link
Contributor

Choose a reason for hiding this comment

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

the original design had the variable name ( label ,... something for a human to identify the item with). variable names are already a bit meh, then we add the repeats ( caused more render issues , added more css for edge cases) adding even more without changing the card design might not be ideal

Copy link
Contributor

Choose a reason for hiding this comment

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

to me the number feels confusing, but i you think most end users will understand the meaning i'm fine with the change

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree with you actually. I was just reinstating previous behavior. I will give at a go to make it more meaningfull

@@ -14,6 +14,10 @@ const props = withDefaults(
const variableKey = computed(() => getKey(props.variable));

const resourcePathId = resourceIdPath(variableKey.value);

function repeats(variable: IVariable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use comupted

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, my mind was still thinking we are working on a list

Copy link

sonarcloud bot commented Sep 26, 2024

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