Skip to content
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

fix(calendar): i18n isn't working in calendar coordinate system #15935

Merged
merged 5 commits into from Oct 25, 2021

Conversation

plainheart
Copy link
Member

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

We introduced the brand-new i18n support in v5.0.0. But it seems that the calendar coordinate system was forgotten and it's still using the hard-coded EN/CN presets.

This PR is intended to implement the i18n feature for the calendar coordinate system and resolve #15932.

But there are two PENDING changes commented on the changed files, which may need some discussion to determine.

Fixed issues

Details

Before: What was the problem?

i18n isn't working in the calendar coordinate system

After: How is it fixed in this PR?

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

Please refer to test/calendar-week.html

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Oct 24, 2021

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

// Or consider adding a new configuration for each locale?
// For example, `time.dayOfWeekShortcut` or `calendar.dayOfWeek`
nameMap = zrUtil.map(
localeModel.get(['time', 'dayOfWeekAbbr']) || [],
Copy link
Contributor

@pissang pissang Oct 25, 2021

Choose a reason for hiding this comment

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

I think providing a short name configuration like dayOfWeekShort is a better and more flexible solution. If i18n doesn't provide this option, we can choose the first charact er of each word.

…yOfWeekShort` doesn't exist in the locale file. 2) `nameMap` can be any registered locale name (case-sensitive).
}
debugger
// Use the capital of `dayOfWeekAbbr` if `dayOfWeekShort` doesn't exist in the locale file.
const dayOfWeekShort = localeModel.get(['time', 'dayOfWeekShort']);
Copy link
Member Author

Choose a reason for hiding this comment

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

@pissang

If i18n doesn't provide this option, we can choose the first charact er of each word.

It merges the default en locale object into the registered locale object. https://github.com/apache/echarts/blob/master/src/core/locale.ts#L62

So we may need to add the new dayOfWeekShort option to each locale file, or at least to the ZH locale file. Otherwise, there will be a type issue in https://github.com/apache/echarts/blob/master/src/core/locale.ts#L80 for missing the dayOfWeekShort option and the following code

map(localeModel.get(['time', 'dayOfWeekAbbr']), val => val[0]);

won't be invoked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a solution. Or we can avoid adding dayOfWeekShort to the en locale? Because it uses the rule that picking the first character. Some non-latin languages may not follow this rule, in which case they need to use this option.

Also, do we need to uppercase the first character? I saw some locale file are not using the uppercase

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we can avoid adding dayOfWeekShort to the en locale?

It should be practical.

Also, do we need to uppercase the first character? I saw some locale file are not using the uppercase

It seems not all languages capitalize the day-of-week or month name. It may be different for each language. See #13055 (comment)

src/component/calendar/CalendarView.ts Outdated Show resolved Hide resolved
pissang
pissang previously approved these changes Oct 25, 2021
@pissang
Copy link
Contributor

pissang commented Oct 25, 2021

LGTM

@plainheart plainheart merged commit 8e683c7 into master Oct 25, 2021
@echarts-bot
Copy link

echarts-bot bot commented Oct 25, 2021

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot Localize Calendar Heatmap
2 participants