Skip to content

Conversation

@ihsaan-ullah
Copy link
Collaborator

@ihsaan-ullah ihsaan-ullah commented Aug 5, 2023

@ mention of reviewers

@Didayolo

A brief description of the purpose of the changes contained in this PR.

If a task has more than one scores, show the primary score (index==0) as bold text

Screenshot 2023-08-05 at 9 38 50 PM

Issues this PR resolves

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • CircleCi tests are passing
  • Ready to merge

@Didayolo
Copy link
Member

Didayolo commented Aug 9, 2023

@ihsaan-ullah Is the primary column always at index 0?

Given that you can select any column as primary using the editor:

Capture d’écran 2023-08-09 à 12 52 54

@ihsaan-ullah
Copy link
Collaborator Author

@ihsaan-ullah Is the primary column always at index 0?

Yes, primary column means index = 0
This can be seen here (although never explained)

primary_index: _.get($('input[name=primary_index]:checked').data(), 'index', 0),

@Didayolo
Copy link
Member

Didayolo commented Aug 9, 2023

@ihsaan-ullah Maybe it is not a problem, or at least could be for a separate PR:

When the other columns are hidden, it still acts as if there are multiple columns and put the scores in bold.

@ihsaan-ullah
Copy link
Collaborator Author

ihsaan-ullah commented Aug 9, 2023

@ihsaan-ullah Maybe it is not a problem, or at least could be for a separate PR:

When the other columns are hidden, it still acts as if there are multiple columns and put the scores in bold.

Can you share a screenshot of this problem. I think this should not be happening because at some point the we have changed the server to not return hidden columns. But this can be investigated further

A separate issue will be better for this

@Didayolo
Copy link
Member

Didayolo commented Aug 9, 2023

Actually it seems that even when I remove the other column, the score is still in bold. I think the problem is actually that the number of scores is fetched from the submission, not from the leaderboard, so you need to upload a new submission to update this.

Another problem with that is that if you edit the leaderboard to change the primary column, the previous submission will still be in bold on the previous primary column.

EDIT: even if I submit AFTER having changed the primary colmumn, it is still the old primary column that is in bold.

To reproduce the different problems mentionned:

  • Upload a submission (and check it on leaderboard)
  • Change the primary column using the editor
  • Upload a submission (and check it on leaderboard)
  • Hide the column using the editor
  • Upload a submission (and check it on leaderboard)
  • Remove the column using the editor
  • Upload a submission (and check it on leaderboard)

@ihsaan-ullah
Copy link
Collaborator Author

@Didayolo

With latest push, the following will happen

  • Bold score is now linked to leaderboard. if you change the primary column from editor, bold columns will change
  • Bold column will appear only if you have more than one columns showing in the leaderboard
  • There was a bug that the hidden column scores were returned from backend. now if you hide a column, its score will not be returned to front end

@Didayolo
Copy link
Member

Thank you for your quick update. I'll test it again asap.

@Didayolo Didayolo self-assigned this Aug 10, 2023
@Didayolo
Copy link
Member

@ihsaan-ullah

Is it normal that you have added index here? Now it is twice:

Capture d’écran 2023-08-10 à 16 49 51

@ihsaan-ullah
Copy link
Collaborator Author

@ihsaan-ullah

Is it normal that you have added index here? Now it is twice:

Capture d’écran 2023-08-10 à 16 49 51

this was by mistake. just removed it in the latest push

@Didayolo Didayolo merged commit 32b4c4b into develop Aug 10, 2023
@Didayolo Didayolo deleted the leaderboard_bold_score branch August 10, 2023 14:56
@Didayolo Didayolo mentioned this pull request Aug 10, 2023
7 tasks
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