Skip to content

Fix(1187): misleading placement of height input#1191

Merged
CodeWithCJ merged 2 commits intoCodeWithCJ:mainfrom
Sim-sat:fix-1187
Apr 25, 2026
Merged

Fix(1187): misleading placement of height input#1191
CodeWithCJ merged 2 commits intoCodeWithCJ:mainfrom
Sim-sat:fix-1187

Conversation

@Sim-sat
Copy link
Copy Markdown
Contributor

@Sim-sat Sim-sat commented Apr 23, 2026

Description

What problem does this PR solve?
Height input is required for tdee and the "update profile" link leads to profile information. But height was in the daily checkin tab. While height can change, nobody will track it daily.

How did you implement the solution?
I moved the height field to the profile information and removed it from daily check in. It's still saved the same way as before. I added the ability for a link to expand any sections in the settings and fixed a UI bug with the warning on small screens.

Linked Issue: Closes #1187

How to Test

  1. Verify height is in profile information and saving works

PR Type

  • Issue (bug fix)
  • New Feature
  • Refactor
  • Documentation

Checklist

All PRs:

  • [MANDATORY - ALL] Integrity & License: I certify this is my own work, free of malicious code, and I agree to the License terms.

New features only:

  • [MANDATORY for new feature] Alignment: I have raised a GitHub issue and it was reviewed/approved by maintainers or it was approved on Discord.

Frontend changes (SparkyFitnessFrontend/ or src/):

  • [MANDATORY for Frontend changes] Quality: I have run pnpm run validate and it passes.
  • [MANDATORY for Frontend changes] Translations: I have only updated the English (en) translation file.

Backend changes (SparkyFitnessServer/):

  • [MANDATORY for Backend changes] Code Quality: I have run typecheck, lint, and tests. New files use TypeScript, new endpoints have Zod schemas, and new endpoints include tests.
  • [MANDATORY for Backend changes] Database Security: I have updated rls_policies.sql for any new user-specific tables.

UI changes (components, screens, pages):

  • [MANDATORY for UI changes] Screenshots: I have attached Before/After screenshots below.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request moves the height input from the check-in form to the profile settings page and refactors the settings accordion structure to support deep linking via URL parameters. It also updates the profile incomplete alert to link directly to the profile section. Feedback focuses on ensuring the height state is correctly synchronized with asynchronous data, preventing the saving of zero values for height, and using the specialized UnitInput component for better unit support.

Comment on lines 42 to 49
const [profileForm, setProfileForm] = useState<ProfileFormState>({
full_name: profile.full_name || '',
phone: profile.phone_number || '',
date_of_birth: profile.date_of_birth || '',
bio: profile.bio || '',
gender: profile.gender || '',
height: heightData?.height || '',
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The profileForm state is initialized with heightData?.height on mount. However, since heightData is fetched asynchronously, it will likely be undefined during the initial render, leaving the height field empty even if a value exists in the database. You should use a useEffect to synchronize the height value once the query data becomes available.

  const [profileForm, setProfileForm] = useState<ProfileFormState>({
    full_name: profile.full_name || '',
    phone: profile.phone_number || '',
    date_of_birth: profile.date_of_birth || '',
    bio: profile.bio || '',
    gender: profile.gender || '',
    height: heightData?.height || '',
  });

  useEffect(() => {
    if (heightData?.height && !profileForm.height) {
      setProfileForm((prev) => ({
        ...prev,
        height: heightData.height,
      }));
    }
  }, [heightData, profileForm.height]);

Comment on lines +62 to +65
const measurementData: UpdateCheckInMeasurementsRequest = {
entry_date: formatDateInUserTimezone(new Date(), 'yyyy-MM-dd'),
height: Number(profileForm.height),
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

When profileForm.height is an empty string, Number(profileForm.height) evaluates to 0. Saving 0 as a height measurement is likely incorrect and could break BMR/TDEE calculations. It is better to send null or skip the measurement update if the field is empty.

      if (profileForm.height) {
        const measurementData: UpdateCheckInMeasurementsRequest = {
          entry_date: formatDateInUserTimezone(new Date(), 'yyyy-MM-dd'),
          height: Number(profileForm.height),
        };
        await saveCheckInMeasurements(measurementData);
      }

Comment on lines +182 to +206
<div>
<Label htmlFor="height">
{t('settings.profileInformation.height', 'Height')}
</Label>
<div className="relative">
<Input
id="height"
type="number"
value={profileForm.height}
onChange={(e) =>
setProfileForm((prev) => ({
...prev,
height: e.target.value,
}))
}
placeholder={t(
'settings.profileInformation.enterHeight',
'Enter your Height'
)}
/>
<span className="absolute right-3 top-1/2 -translate-y-1/2 text-sm text-gray-500 pointer-events-none">
{defaultMeasurementUnit}
</span>
</div>
</div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using a standard Input with type="number" for height loses support for non-metric units like 'Feet & Inches' (ft/in), which is a supported preference in this app. You should use the UnitInput component (with type="height") to ensure consistency with the rest of the application and proper unit handling. Note: You will need to import UnitInput from @/components/ui/UnitInput.

        <div>
          <Label htmlFor="height">
            {t('settings.profileInformation.height', 'Height')}
          </Label>
          <UnitInput
            id="height"
            type="height"
            unit={defaultMeasurementUnit}
            value={profileForm.height.toString()}
            onChange={(val) =>
              setProfileForm((prev) => ({
                ...prev,
                height: val.toString(),
              }))
            }
          />
        </div>

@CodeWithCJ CodeWithCJ merged commit c16fb27 into CodeWithCJ:main Apr 25, 2026
7 checks passed
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.

[Bug]: Height not available in profile setup.

2 participants