Skip to content

Require reference rating #6569

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

Merged
merged 6 commits into from
Aug 1, 2025
Merged

Conversation

fbalda
Copy link
Collaborator

@fbalda fbalda commented Jul 29, 2025

Makes the value of the rating slider on the reference page undefined by default and requires the user to change the value to go to the next step.

Also included are styling changes to better communicate the need to change the slider value:

  • Made track grey by default
  • Made thumb icon a question mark by default
  • Made rail transparent with a black outline (The default color clashes with the grey. As a bonus, this works better with changing colors of the track)

As the rating is now required, the form will display an error message if it isn't set. I defined the key for the translation (profile:leave_reference.rating_required), but the actual texts are still needed
Edit: Text added

closes #6435

I tested this with the local backend by temporarily adding the following code to LeaveReferencePage.tsx right under the useListAvailableReferences call:

if (availableReferences) {
    availableReferences.canWriteFriendReference = true;
}

Logging in with the Aapeli test account and then going to http://localhost:[port]/leave-reference/friend/2 opens the relevant page.

(There is probably a better way to test this, but this worked for me)

Regarding automated tests: I did not add any, as all of the files I touched up until now either had no tests at all or had no tests that were applicable to the code I changed. If desired, I will add some

Web frontend checklist

  • Formatted my code with yarn format
  • There are no warnings from yarn lint --fix
  • There are no console warnings when running the app
  • Added tests where relevant
  • All tests pass
  • Clicked around my changes running locally and it works
  • Checked Desktop, Mobile and Tablet screen sizes

Other
Untick the following if you'd prefer that maintainers don't push commits/merge your branch.

  • A maintainer can push commits to my branch
  • A maintainer can merge my PR (you can also merge after approval)

@fbalda fbalda requested a review from jesseallhands July 29, 2025 13:38
@fbalda fbalda requested a review from nabramow as a code owner July 29, 2025 13:38
Copy link

vercel bot commented Jul 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
couchers ✅ Ready (Inspect) Visit Preview Aug 1, 2025 10:20am

valueLabelFormat={(value) => <SliderLabel value={value} />}
valueLabelFormat={() => <SliderLabel value={value} />}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As this change might be a bit confusing: I added a separate SliderLabel icon when the value is undefined, however the value passed to the valueLabelFormat callback is always a number (as a slider can't have an undefined value). So I'm using the value from the props. AFAIU this doesn't change anything, as the slider re-renders on value change anyway

@fbalda fbalda marked this pull request as draft July 29, 2025 13:48
@fbalda fbalda marked this pull request as ready for review July 29, 2025 13:57
Copy link
Contributor

@jesseallhands jesseallhands left a comment

Choose a reason for hiding this comment

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

seems to be working as expected! Looks fantastic too! I included my suggestion for the error message text (but I'm not sure which json file the text should be stored in - Nicole would know!)

@fbalda fbalda marked this pull request as draft July 30, 2025 08:33
@fbalda fbalda marked this pull request as ready for review July 30, 2025 08:44
jesseallhands
jesseallhands previously approved these changes Jul 30, 2025
Copy link
Contributor

@jesseallhands jesseallhands left a comment

Choose a reason for hiding this comment

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

This is fantastic! Love the new colors too!

LGTM, but have @nabramow take a look before merging

nabramow
nabramow previously approved these changes Jul 30, 2025
Copy link
Member

@nabramow nabramow left a comment

Choose a reason for hiding this comment

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

Just that one small nit, but I tested it with a real reference and was able to properly leave a review and trigger the error message. This looks great!

@@ -35,6 +35,8 @@ import {
} from "routes";
import { theme } from "theme";

const ACCEPTABLE_RATING_THRESHOLD = 0.33;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nabramow I took a bit of an issue with that magic number so I extracted it into a constant. However I was not sure which case to use, there doesn't seem to be a consistent style for constants like this in the codebase. Lmk if this works for you, maybe ideally we should have a short style guide and/or stricter ESlint rules (e.g. naming-convention)? I personally love not having to think about stuff like that while coding 😄

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree building out the eslint rules more has been on my list for awhile! Just has always been more pressing matters ;-). The all caps is good! Feel free to throw that into a small PR for the naming-convention if you want!

Copy link
Member

@nabramow nabramow left a comment

Choose a reason for hiding this comment

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

LGTM!

@fbalda fbalda merged commit 69ebbab into develop Aug 1, 2025
2 checks passed
@fbalda fbalda deleted the web/feature/require-reference-rating branch August 1, 2025 14:58
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.

Make reference slider undefined by default
3 participants