Skip to content

Conversation

@binh-dam-ibigroup
Copy link
Collaborator

This PR adds localization support to OTP-RR.

@binh-dam-ibigroup binh-dam-ibigroup changed the base branch from dev to react-intl July 8, 2021 18:29
@binh-dam-ibigroup binh-dam-ibigroup added the WIP Work in progress label Jul 8, 2021
@binh-dam-ibigroup binh-dam-ibigroup changed the base branch from react-intl to dev July 15, 2021 13:44
Comment on lines 152 to 154
{itinerary.transfers > 1
? <FormattedMessage id='components.ItinerarySummary.transferPlural' />
: <FormattedMessage id='components.ItinerarySummary.transfer' />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use <FormatterPlural> for this case, see https://formatjs.io/docs/react-intl/components#formattedplural

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be addressed as of 4bbbe49

Comment on lines 129 to 143
<FormattedNumber
value={minTotalFare / 100}
style='currency'
currencyDisplay='narrowSymbol'
currency={currency}
/>
{minTotalFare !== maxTotalFare &&
<span> -
<FormattedNumber
value={maxTotalFare / 100}
style='currency'
currencyDisplay='narrowSymbol'
currency={currency}
/>
</span>}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a concat; it needs to be turned into a conditional message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be addressed as of 4bbbe49

Comment on lines 119 to 146
{Math.round(caloriesBurned)} Cals
{Math.round(caloriesBurned)} <FormattedMessage id='components.ItinerarySummary.calories' />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found multiple spellings for the Calories abbreviation:

  • This component (used in mobile view of itinerary results) displays "Cals" (as in "100 Cals").
  • The tabbed itinerary summary (in desktop view) displays "Cal" (as in "100 Cal").
    I am in favor of merging the two and say, in English, "100 Cal".

Copy link
Contributor

Choose a reason for hiding this comment

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

I have changed the components I touched to "Cal" as of 4bbbe49

<Detail>
{coreUtils.time.formatTime(itinerary.startTime, timeOptions)} - {coreUtils.time.formatTime(itinerary.endTime, timeOptions)}
<FormattedTime
startTime={itinerary.startTime}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to pass a message id.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated as of 4bbbe49

i18n/en-US.yml Outdated
true {{minTotalFare} - {maxTotalFare}}
false {{minTotalFare}}
}}"
transfers: "{transfers} {transfers, plural, =0 {Transfer} other {Transfers}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is an issue with this expression:
image

In example.js, set:

  ItineraryBody: LineItinerary, // (add import)

and make an itinerary search in mobile view.

Copy link
Contributor

@philip-cline philip-cline Jul 20, 2021

Choose a reason for hiding this comment

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

This should be addressed in the latest commit (1fbee66)

image

* component. The monitoredTrip param can be undefined.
*/
export default function baseRenderer (monitoredTrip) {
const intl = useIntl()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tricky: baseRenderer is not a component and not a functional component, so this call fails when rendering a trip detail page.

Copy link
Contributor

@philip-cline philip-cline Aug 4, 2021

Choose a reason for hiding this comment

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

This would hold for changes to active-trip-renderer as well:

export default function activeTripRenderer ({
monitoredTrip,
onTimeThresholdSeconds,
timeFormat
}) {
const intl = useIntl()

Copy link
Contributor

Choose a reason for hiding this comment

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

Both should be addressed in 6c54a1e

i18n/en-US.yml Outdated
BaseRenderer:
bodyDefault: Unknown Trip State
lastCheckedDefaultText: Last checked time unknown
LastCheckedText: "Last checked: {formattedDuration} ago"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be lastCheckedText (lower case l)

Copy link
Contributor

Choose a reason for hiding this comment

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

addressed in 6c54a1e

i18n/en-US.yml Outdated
Comment on lines 47 to 48
true {Unsnooze trip analysis}
other {Snooze for rest of today}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These should probably two separate messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Separated into tripIsNotSnoozed and tripIsSnoozed messages in 6c54a1e

i18n/en-US.yml Outdated
Comment on lines 464 to 465
departure: DEPARTURE
status: STATUS
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@philip-cline These table headings should be first letter capitalized here. To render them all-caps, there should be some CSS for the corresponding elements in viewers.css.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO, departure and status should be under components.PatternRow, or is there a reason to place them under common?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these addressed in 8a8cfae, I had put the messages under common in case they would be used in the future by another component.

)
// Add tall class to account for vertical centering if there is only
// one line in the label (default is 2).
const addClass = messages.label.match(/\n/) ? '' : ' tall'
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the appropriate way to apply styles and formatting here? Right now we're relying on a multi line string in the i18n files

@binh-dam-ibigroup binh-dam-ibigroup merged commit bf1d2d6 into dev Nov 24, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 3.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@binh-dam-ibigroup binh-dam-ibigroup deleted the react-intl-ramp-up branch December 18, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants