🔨 change fallback chain for display.name and titlePublic #3016
Conversation
There was a problem hiding this comment.
Looks good to me!
Just a few minor comments about how to show the extra fields in Grapher's data tabels.
For table captions, the extra fields and the unit are both shown in grey and not separated from each other, which is a bit weird:
The extra fields make column names a bit crowded, not sure we want to show it here? If there's been a lot of discussion this, please ignore me.
| attributionShort: this.def.presentation?.attributionShort, | ||
| titleVariant: this.def.presentation?.titleVariant, | ||
| } | ||
| : { title: this.display?.name ?? this.name ?? "" } |
There was a problem hiding this comment.
|| instead ?? would be better here because sometimes display.name is an empty string, and then the title would be empty
| attributionShort: this.def.presentation?.attributionShort, | ||
| titleVariant: this.def.presentation?.titleVariant, | ||
| } | ||
| : { title: this.display?.name ?? this.name ?? "" } | ||
| } | ||
|
|
||
| @imemo get nonEmptyDisplayName(): string { |
There was a problem hiding this comment.
After your changes, this is the only place where nonEmptyDisplayName is used now:
I think it would make sense to replace this with titlePublicOrDisplayName and then delete nonEmptyDisplayName
danyx23
force-pushed
the
change-display-name-vs-title-public-fallbacks
branch
from
e1fa94b to
ec5ae70
Compare
|
Thanks a lot, @sophiamersmann, very good points. I left the table as is for now since we kinda need the additional fragments with the title public but maybe we can tweak it some time soon with Marwa. |
This PR changes the fallback chain for title-ish strings according to #3017