raidboss: localized job names #5930
Conversation
| import { OutputStringsParamObject } from './trigger'; | ||
|
|
||
| export type BasePartyMemberParamObject = { | ||
| role?: Role; | ||
| role?: string; |
There was a problem hiding this comment.
Why change the type of the Role? I think you should keep it remained but add none into Role instead.
There was a problem hiding this comment.
Because 'role' is not a fixed string after localization.
There was a problem hiding this comment.
Hmmm, in this case, I would recommend to add an extra field to store the localized name rather than changing the original one. What do you think?
There was a problem hiding this comment.
I agree with your idea. Initially, I also added a new attribute.
However, later I thought that existing users might have saved options with 'role' as the key.
For instance, if I added a new one, let's say 'roleName,' how should we handle users who previously saved options with 'role'?
Do we need to delete the 'role' option, and if not deleted, this option would only be effective for English language users, while 'roleName' would only be effective for non-English users. This would be quite awkward.
I haven't come up with a good solution.
There was a problem hiding this comment.
I think this has only been out briefly, so I think it's fine to change and not worry about existing users.
Do we need to delete the 'role' option, and if not deleted, this option would only be effective for English language users, while 'roleName' would only be effective for non-English users. This would be quite awkward.
Yeah, this is my worry as well. I think I would prefer to have role localized and not need two of them. I don't think that non-English users need to be able to access the English role name. (It's sort of the same thing with job and jobAbbr too.)
resources/party.ts
Outdated
| const jobAbbr = jobLocalizedAbbr[job]?.[Options.AlertsLanguage ?? 'en'] ?? job; | ||
| const jobFull = jobLocalizedFull[job]?.[Options.AlertsLanguage ?? 'en'] ?? job; | ||
| const role = Util.jobToRole(job); | ||
| const roleName = roleLocalized[role]?.[Options.AlertsLanguage ?? 'en'] ?? role; |
There was a problem hiding this comment.
| const jobAbbr = jobLocalizedAbbr[job]?.[Options.AlertsLanguage ?? 'en'] ?? job; | |
| const jobFull = jobLocalizedFull[job]?.[Options.AlertsLanguage ?? 'en'] ?? job; | |
| const role = Util.jobToRole(job); | |
| const roleName = roleLocalized[role]?.[Options.AlertsLanguage ?? 'en'] ?? role; | |
| const lang = this.options.DisplayLanguage; | |
| const jobAbbr = jobLocalizedAbbr[job]?.[lang] ?? job; | |
| const jobFull = jobLocalizedFull[job]?.[lang] ?? job; | |
| const role = Util.jobToRole(job); | |
| const roleName = roleLocalized[role]?.[lang] ?? role; |
The Options object is the default and not (necessarily?) the one with user config applied. It's better to use the one passed into the constructor here. I think DisplayLanguage is more correct here as this.options doesn't know about AlertsLanguage for oopsy. I think you'll need to add it (or both) to the PartyTrackerOptions type.
There was a problem hiding this comment.
(sorry for the complication, I forgot this part)
| @@ -17,7 +18,7 @@ export interface PartyMemberParamObject | |||
| } | |||
|
|
|||
| // This is a partial interface of both RaidbossOptions and OopsyOptions. | |||
| export interface PartyTrackerOptions { | |||
| export interface PartyTrackerOptions extends BaseOptions { | |||
There was a problem hiding this comment.
Ah this is a good idea, thanks!
Please provide any comments.