Add preferred language, unit, and time format #42
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new component, UserPreferences, into the user registration process. The SignUpForm.astro file is updated to import and include UserPreferences immediately after the ContactInformation component. The UserPreferences component provides dropdowns for selecting language, measurement unit, and time format, based on predefined interfaces and constants. It also runs a script on page load to initialize these selections according to the user's browser language and region. Existing functionality remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Browser as Browser
participant SignUp as SignUpForm
participant Pref as UserPreferences
User->>Browser: Load registration page
Browser->>SignUp: Render SignUpForm
SignUp->>Pref: Include UserPreferences component
Pref->>Browser: Retrieve browser language & region
Browser-->>Pref: Return language and region info
Pref->>Pref: Set default selections for language, unit, and time format
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@geographyclub, mind taking a look and letting me know if it works for Canadians? = ) The whole sign up doesn't work yet. This just adds the inputs. Decided against querying the database as we don't have a lot of options yet and this would require creating more tables. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/user-preferences/UserPreferences.astro (2)
2-19: Consider enhancing type definitions and configuration management.
- The Language interface could be more comprehensive for future internationalization:
- Consider using an enum for unit codes to improve type safety
- Move constants to separate configuration files for better maintainability
+// src/config/i18n.ts +export interface Language { + code: string; + name: string; + direction?: 'ltr' | 'rtl'; + isDefault?: boolean; +} +export const languages: Language[] = [ + { code: "en", name: "English", isDefault: true } +]; +// src/config/units.ts +export enum UnitSystem { + METRIC = 'metric', + IMPERIAL = 'imperial' +} +export interface Unit { + code: UnitSystem; + name: string; +} +export const units: Unit[] = [ + { code: UnitSystem.METRIC, name: "Metric (°C, meters, liters)" }, + { code: UnitSystem.IMPERIAL, name: "Imperial (°F, miles, gallons)" } +];
22-63: Enhance accessibility and user experience.Consider the following improvements:
- Add aria-labels and aria-required for better screen reader support
- Add placeholder options
- Include validation messages
<select id="language" name="language" + aria-label="Select your preferred language" + aria-required="true" class="w-full px-4 py-2 border border-slate-300 rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500" required > + <option value="" disabled selected>Select a language...</option> {languages.map(lang => ( <option value={lang.code}> {lang.name} </option> ))} </select> +<span class="text-sm text-red-600 hidden" id="language-error">Please select a language</span> <select id="unit" name="unit" + aria-label="Select your preferred measurement unit" + aria-required="true" class="w-full px-4 py-2 border border-slate-300 rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500" required > + <option value="" disabled selected>Select measurement units...</option> {units.map(unit => ( <option value={unit.code}> {unit.name} </option> ))} </select> +<span class="text-sm text-red-600 hidden" id="unit-error">Please select measurement units</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/SignUpForm.astro(2 hunks)src/components/user-preferences/UserPreferences.astro(1 hunks)
🔇 Additional comments (2)
src/components/SignUpForm.astro (2)
3-3: LGTM!The import statement is correctly placed and follows the component structure.
15-15: LGTM!The UserPreferences component is logically placed after contact information and before notification preferences.
|
Looks like we might be able to infer the user's time preference using toLocaleString for a date and seeing if there is a PM/AM or not.
|
jsolly
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/user-preferences/UserPreferences.astro (1)
80-122: 🛠️ Refactor suggestionImprove script robustness and maintainability.
The initialization script needs improvements in error handling, configuration management, and race condition prevention.
+// Move to src/config/preferences.ts +const IMPERIAL_REGIONS = ['US', 'MM', 'LR'] as const; +type ImperialRegion = typeof IMPERIAL_REGIONS[number]; + +interface TimeFormat { + readonly code: '12h' | '24h'; + readonly example: string; +} + +const DEFAULT_PREFERENCES = { + language: 'en', + unit: 'metric', + timeFormat: '24h' +} as const; <script> + // Utility function to safely get DOM elements + function getElement<T extends HTMLElement>(id: string): T | null { + const element = document.getElementById(id) as T; + if (!element) { + console.error(`Element with id '${id}' not found`); + } + return element; + } + + // Determine unit system based on region + function getUnitSystemForRegion(region: string): 'metric' | 'imperial' { + return IMPERIAL_REGIONS.includes(region as ImperialRegion) + ? 'imperial' + : 'metric'; + } + + // Detect time format preference + function detectTimeFormat(): '12h' | '24h' { + try { + const sampleDate = new Date(); + return sampleDate.toLocaleTimeString() + .match(/AM|PM/i) ? '12h' : '24h'; + } catch { + return DEFAULT_PREFERENCES.timeFormat; + } + } function setInitialPreferences() { - const languageSelect = document.getElementById('language') as HTMLSelectElement; - const unitSelect = document.getElementById('unit') as HTMLSelectElement; - const timeFormatSelect = document.getElementById('timeFormat') as HTMLSelectElement; + const languageSelect = getElement<HTMLSelectElement>('language'); + const unitSelect = getElement<HTMLSelectElement>('unit'); + const timeFormatSelect = getElement<HTMLSelectElement>('timeFormat'); if (languageSelect && unitSelect && timeFormatSelect) { try { - const [lang, region] = (navigator.language || 'en-US').split('-'); + // Get browser's language and region + const [lang = '', region = ''] = (navigator.language || '') + .split('-'); - if (lang && ['en'].includes(lang.toLowerCase())) { - languageSelect.value = lang.toLowerCase(); - } else { - languageSelect.value = 'en'; - } + // Set language if available, otherwise use default + languageSelect.value = languages + .some(l => l.code === lang.toLowerCase()) + ? lang.toLowerCase() + : DEFAULT_PREFERENCES.language; - if (region && !['US', 'MM', 'LR'].includes(region.toUpperCase())) { - unitSelect.value = 'metric'; - } else { - unitSelect.value = 'imperial'; - } + // Set unit based on region + unitSelect.value = getUnitSystemForRegion(region.toUpperCase()); - const sampleDate = new Date(); - const timeString = sampleDate.toLocaleTimeString(); - timeFormatSelect.value = timeString.match(/AM|PM/i) ? '12h' : '24h'; + // Set time format + timeFormatSelect.value = detectTimeFormat(); } catch (error) { - languageSelect.value = 'en'; - unitSelect.value = 'imperial'; - timeFormatSelect.value = '24h'; + console.error('Error setting initial preferences:', error); + // Use defaults if anything goes wrong + languageSelect.value = DEFAULT_PREFERENCES.language; + unitSelect.value = DEFAULT_PREFERENCES.unit; + timeFormatSelect.value = DEFAULT_PREFERENCES.timeFormat; } } } - document.addEventListener('DOMContentLoaded', setInitialPreferences); + // Run immediately if DOM is already loaded, otherwise wait for DOMContentLoaded + if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', setInitialPreferences); + } else { + setInitialPreferences(); + } </script>
🧹 Nitpick comments (3)
src/components/user-preferences/UserPreferences.astro (3)
2-10: Consider enhancing interfaces with additional type safety.The interfaces could benefit from more specific types and readonly properties to prevent accidental modifications.
-interface Language { - code: string; - name: string; +interface Language { + readonly code: 'en' | 'fr' | 'es'; // List supported languages + readonly name: string; } -interface Unit { - code: string; - name: string; +interface Unit { + readonly code: 'metric' | 'imperial'; + readonly name: string; }
12-19: Move configuration to a separate file.Consider extracting these constants to a dedicated configuration file for better maintainability.
Create a new file
src/config/preferences.ts:export interface Language { readonly code: 'en' | 'fr' | 'es'; readonly name: string; } export interface Unit { readonly code: 'metric' | 'imperial'; readonly name: string; } export const languages: readonly Language[] = [ { code: "en", name: "English" } ] as const; export const units: readonly Unit[] = [ { code: "metric", name: "Metric (°C, meters, liters)" }, { code: "imperial", name: "Imperial (°F, miles, gallons)" } ] as const;
30-35: Enhance accessibility with ARIA attributes.Add aria-label attributes to improve accessibility for screen readers.
<select id="language" name="language" + aria-label="Select your preferred language" class="w-full px-4 py-2 border border-slate-300 rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500" required > <select id="unit" name="unit" + aria-label="Select your preferred measurement units" class="w-full px-4 py-2 border border-slate-300 rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500" required > <select id="timeFormat" name="timeFormat" + aria-label="Select your preferred time format" class="w-full px-4 py-2 border border-slate-300 rounded-lg focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500" required >Also applies to: 49-54, 67-72
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/user-preferences/UserPreferences.astro (4)
2-10: Consider consolidating identical interfaces.The
LanguageandUnitinterfaces share the same structure. Consider using a generic interface to reduce duplication.-interface Language { - code: string; - name: string; -} - -interface Unit { - code: string; - name: string; -} +interface Option { + code: string; + name: string; +} + +type Language = Option; +type Unit = Option;
109-113: Improve region-based unit selection logic.The current logic for determining imperial regions is inverted, making it harder to read and maintain.
- if (region && !['US', 'MM', 'LR'].includes(region.toUpperCase())) { - unitSelect.value = 'metric'; - } else { - unitSelect.value = 'imperial'; - } + const IMPERIAL_REGIONS = ['US', 'MM', 'LR']; + const regionUnit = region && IMPERIAL_REGIONS.includes(region.toUpperCase()) + ? 'imperial' + : 'metric'; + unitSelect.value = regionUnit;
116-119: Enhance time format detection.The current time format detection might not work reliably across all locales. Consider using
Intl.DateTimeFormatfor more robust detection.- const sampleDate = new Date(); - const timeString = sampleDate.toLocaleTimeString(); - // If AM/PM is present in the time string, use 12h format - timeFormatSelect.value = timeString.match(/AM|PM/i) ? '12h' : '24h'; + const formatter = new Intl.DateTimeFormat(navigator.language, { + hour: 'numeric', + hour12: undefined + }); + const is12Hour = formatter.resolvedOptions().hour12 ?? true; + timeFormatSelect.value = is12Hour ? '12h' : '24h';
130-131: Consider race conditions in script initialization.The script might miss the DOMContentLoaded event if it runs after the event has already fired.
- // Run on page load - document.addEventListener('DOMContentLoaded', setInitialPreferences); + // Run immediately if DOM is already loaded, otherwise wait for DOMContentLoaded + if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', setInitialPreferences); + } else { + setInitialPreferences(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/user-preferences/UserPreferences.astro(1 hunks)
🔇 Additional comments (3)
src/components/user-preferences/UserPreferences.astro (3)
39-44: Add key prop to mapped elements.React/Astro requires a unique key prop when mapping over arrays to render elements.
61-66: Add key prop to mapped elements.React/Astro requires a unique key prop when mapping over arrays to render elements.
69-86: LGTM! Well-structured time format selection.The implementation is consistent with other selections and includes proper accessibility attributes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/user-preferences/UserPreferences.astro (2)
81-84: Define time format options as a constant.For consistency with languages and units, consider defining time format options as a constant.
+const timeFormats: Option[] = [ + { code: "24h", name: "24h (eg. 23:00)" }, + { code: "12h", name: "12h (eg. 11:00 PM)" } +]; -<option value="" disabled selected>Select time format...</option> -<option value="24h">24h (eg. 23:00)</option> -<option value="12h">12h (eg. 11:00 PM)</option> +<option value="" disabled selected>Select time format...</option> +{timeFormats.map(format => ( + <option key={format.code} value={format.code}> + {format.name} + </option> +))}
95-95: Move imperial countries to constants.For better maintainability, move the imperial countries list to the constants section at the top of the file.
+const IMPERIAL_COUNTRIES = ['US', 'MM', 'LR'] as const; +type ImperialCountry = typeof IMPERIAL_COUNTRIES[number]; -const imperialCountries = ['US', 'MM', 'LR']; +const imperialCountries = IMPERIAL_COUNTRIES;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/user-preferences/UserPreferences.astro(1 hunks)
🔇 Additional comments (4)
src/components/user-preferences/UserPreferences.astro (4)
38-43: Add key prop to mapped elements.React/Astro requires a unique key prop when mapping over arrays to render elements.
{languages.map(lang => ( - <option value={lang.code}> + <option key={lang.code} value={lang.code}> {lang.name} </option> ))}
60-65: Add key prop to mapped elements.React/Astro requires a unique key prop when mapping over arrays to render elements.
{units.map(unit => ( - <option value={unit.code}> + <option key={unit.code} value={unit.code}> {unit.name} </option> ))}
3-18: Consider adding more language options for international users.The current implementation only supports English, which might limit the application's accessibility for international users. Consider adding support for more languages.
Would you like me to help implement support for additional languages? I can provide a list of commonly used languages and their corresponding codes.
Also, consider making the unit descriptions more consistent:
const units: Unit[] = [ - { code: "metric", name: "Metric (°C, meters, liters)" }, - { code: "imperial", name: "Imperial (°F, miles, gallons)" }, + { code: "metric", name: "Metric (Celsius, Meters, Liters)" }, + { code: "imperial", name: "Imperial (Fahrenheit, Miles, Gallons)" }, ];✅ Verification successful
Error: No shell scripts or web queries found
Internationalization & Consistency in Display Text
The type definitions and constant arrays in the file are correctly implemented. However, note the following suggestions for improvement:
- Language Options: Currently, only English is supported. For better internationalization, consider expanding the
languageslist with additional language options.- Unit Descriptions: Updating the unit names for consistency (e.g., using “Celsius”, “Meters”, “Liters” and “Fahrenheit”, “Miles”, “Gallons”) can improve clarity and consistency.
These suggestions do not impact functionality but could improve user experience.
132-133: 🛠️ Refactor suggestionConsider DOMContentLoaded event.
The script might run before the DOM is fully loaded. Consider using the DOMContentLoaded event.
-// Run initialization immediately -setInitialPreferences(); +// Run immediately if DOM is already loaded, otherwise wait for DOMContentLoaded +if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', setInitialPreferences); +} else { + setInitialPreferences(); +}Likely invalid or redundant comment.
|
Looks good from my end! If there are issues, we can open a new PR. |
Summary by CodeRabbit
Summary by CodeRabbit